On Sat, 2021-01-09 at 19:26 +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <[email protected]>
>
> There is an effort to convert .ndo_get_stats64 to sleepable context,
> and
> for that to work, we need to prevent callers of dev_get_stats from
> using
> atomic locking.
>
> The bonding driver retrieves its statistics recursively from its
> lower
> interfaces, with additional care to only count packets sent/received
> while those lowers were actually enslaved to the bond - see commit
> 5f0c5f73e5ef ("bonding: make global bonding stats more reliable").
>
> Since commit 87163ef9cda7 ("bonding: remove last users of bond->lock
> and
> bond->lock itself"), the bonding driver uses the following protection
> for its array of slaves: RCU for readers and rtnl_mutex for updaters.
>
> The aforementioned commit removed an interesting comment:
>
> /* [...] we can't hold bond->lock [...] because we'll
> * deadlock. The only solution is to rely on the fact
> * that we're under rtnl_lock here, and the slaves
> * list won't change. This doesn't solve the problem
> * of setting the slave's MTU while it is
> * transmitting, but the assumption is that the base
> * driver can handle that.
> *
> * TODO: figure out a way to safely iterate the slaves
> * list, but without holding a lock around the actual
> * call to the base driver.
> */
>
> The above summarizes pretty well the challenges we have with nested
> bonding interfaces (bond over bond over bond over...) and locking for
> their slaves.
>
> To solve the nesting problem, the simple way is to not hold any locks
> when recursing into the slave netdev operation. We can "cheat" and
> use
> dev_hold to take a reference on the slave net_device, which is enough
> to
> ensure that netdev_wait_allrefs() waits until we finish, and the
> kernel
> won't fault.
>
> However, the slave structure might no longer be valid, just its
> associated net_device. So we need to do some more work to ensure that
> the slave exists after we took the statistics, and if it still does,
> reapply the logic from Andy's commit 5f0c5f73e5ef.
>
> Tested using the following two scripts running in parallel:
>
> #!/bin/bash
>
> while :; do
> ip link del bond0
> ip link del bond1
> ip link add bond0 type bond mode 802.3ad
> ip link add bond1 type bond mode 802.3ad
> ip link set sw0p1 down && ip link set sw0p1 master
> bond0 && ip link set sw0p1 up
> ip link set sw0p2 down && ip link set sw0p2 master
> bond0 && ip link set sw0p2 up
> ip link set sw0p3 down && ip link set sw0p3 master
> bond0 && ip link set sw0p3 up
> ip link set bond0 down && ip link set bond0 master
> bond1 && ip link set bond0 up
> ip link set sw1p1 down && ip link set sw1p1 master
> bond1 && ip link set sw1p1 up
> ip link set bond1 up
> ip -s -s link show
> cat /sys/class/net/bond1/statistics/*
> done
>
> #!/bin/bash
>
> while :; do
> echo spi2.0 > /sys/bus/spi/drivers/sja1105/unbind
> echo spi2.0 > /sys/bus/spi/drivers/sja1105/bind
> sleep 30
> done
>
> where the sja1105 driver was explicitly modified for the purpose of
> this
> test to have a msleep(500) in its .ndo_get_stats64 method, to catch
> some
> more potential races.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
>
[...]
>
> +/* Helpers for reference counting the struct net_device behind the
> bond slaves.
> + * These can be used to propagate the net_device_ops from the bond
> to the
> + * slaves while not holding rcu_read_lock() or the rtnl_mutex.
> + */
> +struct bonding_slave_dev {
> + struct net_device *ndev;
> + struct list_head list;
> +};
> +
> +static inline void bond_put_slaves(struct list_head *slaves)
> +{
> + struct bonding_slave_dev *s, *tmp;
> +
> + list_for_each_entry_safe(s, tmp, slaves, list) {
> + dev_put(s->ndev);
> + list_del(&s->list);
> + kfree(s);
> + }
> +}
> +
> +static inline int bond_get_slaves(struct bonding *bond,
> + struct list_head *slaves,
> + int *num_slaves)
> +{
> + struct list_head *iter;
> + struct slave *slave;
> +
> + INIT_LIST_HEAD(slaves);
> + *num_slaves = 0;
> +
> + rcu_read_lock();
> +
> + bond_for_each_slave_rcu(bond, slave, iter) {
> + struct bonding_slave_dev *s;
> +
> + s = kzalloc(sizeof(*s), GFP_ATOMIC);
GFP_ATOMIC is a little bit aggressive especially when user daemons are
periodically reading stats. This can be avoided.
You can pre-allocate with GFP_KERNEL an array with an "approximate"
size.
then fill the array up with whatever slaves the the bond has at that
moment, num_of_slaves can be less, equal or more than the array you
just allocated but we shouldn't care ..
something like:
rcu_read_lock()
nslaves = bond_get_num_slaves();
rcu_read_unlock()
sarray = kcalloc(nslaves, sizeof(struct bonding_slave_dev),
GFP_KERNEL);
rcu_read_lock();
bond_fill_slaves_array(bond, sarray); // also do: dev_hold()
rcu_read_unlock();
bond_get_slaves_array_stats(sarray);
bond_put_slaves_array(sarray);
> + if (!s) {
> + rcu_read_unlock();
> + bond_put_slaves(slaves);
> + return -ENOMEM;
> + }
> +
> + s->ndev = slave->dev;
> + dev_hold(s->ndev);
> + list_add_tail(&s->list, slaves);
> + (*num_slaves)++;
> + }
> +
> + rcu_read_unlock();
> +
> + return 0;
> +}
> +
> #define BOND_PRI_RESELECT_ALWAYS 0
> #define BOND_PRI_RESELECT_BETTER 1
> #define BOND_PRI_RESELECT_FAILURE 2
