On Wed, Jan 31, 2024 at 01:35:43PM +0100, Tobias Waldekranz wrote:
> When adding/removing a port to/from a bridge, the port must be brought
> up to speed with the current state of the bridge. This is done by
> replaying all relevant events, directly to the port in question.
>
> In some situations, specifically when replaying the MDB, this process
> may race against new events that are generated concurrently.
>
> So the bridge must ensure that the event is not already pending on the
> deferred queue. switchdev_port_obj_is_deferred answers this question.
>
> Signed-off-by: Tobias Waldekranz <tobias@xxxxxxxxxxxxxx>
I don't see great value in splitting this patch in (1) unused helpers
(2) actual fix that uses them. Especially since it creates confusion -
it is nowhere made clear in this commit message that it is just
preparatory work.
> ---
> include/net/switchdev.h | 3 ++
> net/switchdev/switchdev.c | 61 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index a43062d4c734..538851a93d9e 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -308,6 +308,9 @@ void switchdev_deferred_process(void);
> int switchdev_port_attr_set(struct net_device *dev,
> const struct switchdev_attr *attr,
> struct netlink_ext_ack *extack);
> +bool switchdev_port_obj_is_deferred(struct net_device *dev,
> + enum switchdev_notifier_type nt,
> + const struct switchdev_obj *obj);
I think this is missing a shim definition for when CONFIG_NET_SWITCHDEV
is disabled.
> int switchdev_port_obj_add(struct net_device *dev,
> const struct switchdev_obj *obj,
> struct netlink_ext_ack *extack);
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 5b045284849e..40bb17c7fdbf 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -19,6 +19,35 @@
> #include <linux/rtnetlink.h>
> #include <net/switchdev.h>
>
> +static bool switchdev_obj_eq(const struct switchdev_obj *a,
> + const struct switchdev_obj *b)
> +{
> + const struct switchdev_obj_port_vlan *va, *vb;
> + const struct switchdev_obj_port_mdb *ma, *mb;
> +
> + if (a->id != b->id || a->orig_dev != b->orig_dev)
> + return false;
> +
> + switch (a->id) {
> + case SWITCHDEV_OBJ_ID_PORT_VLAN:
> + va = SWITCHDEV_OBJ_PORT_VLAN(a);
> + vb = SWITCHDEV_OBJ_PORT_VLAN(b);
> + return va->flags == vb->flags &&
> + va->vid == vb->vid &&
> + va->changed == vb->changed;
> + case SWITCHDEV_OBJ_ID_PORT_MDB:
> + case SWITCHDEV_OBJ_ID_HOST_MDB:
> + ma = SWITCHDEV_OBJ_PORT_MDB(a);
> + mb = SWITCHDEV_OBJ_PORT_MDB(b);
> + return ma->vid == mb->vid &&
> + !memcmp(ma->addr, mb->addr, sizeof(ma->addr));
ether_addr_equal().
> + default:
> + break;
Does C allow you to not return anything here?
> + }
> +
> + BUG();
> +}
> +
> static LIST_HEAD(deferred);
> static DEFINE_SPINLOCK(deferred_lock);
>
> @@ -307,6 +336,38 @@ int switchdev_port_obj_del(struct net_device *dev,
> }
> EXPORT_SYMBOL_GPL(switchdev_port_obj_del);
>
> +bool switchdev_port_obj_is_deferred(struct net_device *dev,
> + enum switchdev_notifier_type nt,
> + const struct switchdev_obj *obj)
A kernel-doc comment would be great. It looks like it's not returning
whether the port object is deferred, but whether the _action_ given by
@nt on the @obj is deferred. This further distinguishes between deferred
additions and deferred removals.
> +{
> + struct switchdev_deferred_item *dfitem;
> + bool found = false;
> +
> + ASSERT_RTNL();
Why does rtnl_lock() have to be held? To fully allow switchdev_deferred_process()
to run to completion, aka its dfitem->func() as well?
> +
> + spin_lock_bh(&deferred_lock);
> +
> + list_for_each_entry(dfitem, &deferred, list) {
> + if (dfitem->dev != dev)
> + continue;
> +
> + if ((dfitem->func == switchdev_port_obj_add_deferred &&
> + nt == SWITCHDEV_PORT_OBJ_ADD) ||
> + (dfitem->func == switchdev_port_obj_del_deferred &&
> + nt == SWITCHDEV_PORT_OBJ_DEL)) {
> + if (switchdev_obj_eq((const void *)dfitem->data, obj)) {
> + found = true;
> + break;
> + }
> + }
> + }
> +
> + spin_unlock_bh(&deferred_lock);
> +
> + return found;
> +}
> +EXPORT_SYMBOL_GPL(switchdev_port_obj_is_deferred);
> +
> static ATOMIC_NOTIFIER_HEAD(switchdev_notif_chain);
> static BLOCKING_NOTIFIER_HEAD(switchdev_blocking_notif_chain);
>
> --
> 2.34.1
>
