Re: [patch net-next v5 3/8] switchdev: allow caller to explicitly request attr_set as deferred

[John Fastabend <john.fastabend@gmail.com>]

On 15-10-14 10:40 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Caller should know if he can call attr_set directly (when holding RTNL)
> or if he has to defer the att_set processing for later.
> 
> This also allows drivers to sleep inside attr_set and report operation
> status back to switchdev core. Switchdev core then warns if status is
> not ok, instead of silent errors happening in drivers.
> 
> Benefit from newly introduced switchdev deferred ops infrastructure.
> 

A nit but the patch description should note your setting the defer bit
on the bridge set state.

> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/switchdev.h   |   1 +
>  net/bridge/br_stp.c       |   3 +-
>  net/switchdev/switchdev.c | 108 ++++++++++++++++++----------------------------
>  3 files changed, 46 insertions(+), 66 deletions(-)
> 
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index d1c7f90..f7de6f8 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -17,6 +17,7 @@
>  
>  #define SWITCHDEV_F_NO_RECURSE		BIT(0)
>  #define SWITCHDEV_F_SKIP_EOPNOTSUPP	BIT(1)
> +#define SWITCHDEV_F_DEFER		BIT(2)
>  
>  struct switchdev_trans_item {
>  	struct list_head list;
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index db6d243de..80c34d7 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -41,13 +41,14 @@ void br_set_state(struct net_bridge_port *p, unsigned int state)
>  {
>  	struct switchdev_attr attr = {
>  		.id = SWITCHDEV_ATTR_ID_PORT_STP_STATE,
> +		.flags = SWITCHDEV_F_DEFER,
>  		.u.stp_state = state,
>  	};


This creates a possible race (with 6/8) I think, please check!

In del_nbp() we call br_stp_disable_port() to set the port state
to BR_STATE_DISABLE and disabling learning events. But with this
patch it can be deferred. Also note the STP agent may be in userspace
which actually seems more likely the case because you likely want to
run some more modern variant of STP than the kernel supports.

So at some point in the future the driver will turn off learning. At
the same time we call br_fdb_delete_by_port which calls a deferred
set of fdb deletes.

I don't see how you guarantee learning is off before you start doing
the deletes here and possibly learning new addresses after the software
side believes the port is down.

So

   br_stp_disable_port
                           br_fdb_delete_by_port
                           {fdb_del_external_learn}
   [hw learns a fdb]
   [hw disables learning]

What stops this from happening?