Re: [PATCH RFC net-next 07/10] net: dsa: mv88e6xxx: Track bridge mdb objects

[Joseph Huang]

On 4/29/2024 8:59 PM, Vladimir Oltean wrote:
> On Mon, Apr 29, 2024 at 06:07:25PM -0400, Joseph Huang wrote:
> > Something like this (some layers omitted for brevity)?
> >
> >                                        +br_iterator
> >                                        |  for each mdb
> >                                        |    _br_switchdev_mdb_notify
> > rtnl_lock                             |      without F_DEFER flag
> >   |                                    |      |
> >   +switchdev_port_attr_set_deferred    |      +switchdev_port_obj_notify
> >     |                                  |        |
> >     +dsa_port_mrouter                  |        +dsa_user_port_obj_a/d
> >       |                                |          |
> >       +mv88e6xxx_port_mrouter----------+          +mv88e6xxx_port_obj_a/d
> >                                          |
> >   +--------------------------------------+
> >   |
> > rtnl_unlock
>
> At a _very_ superficial glance, I don't think you are properly
> accounting for the fact that even with rtnl_lock() held, there are still
> SWITCHDEV_OBJ_ID_PORT_MDB events which may be pending on the switchdev
> chain. Without a switchdev_deferred_process() flush call, you won't be
> getting rid of them, so when you rtnl_unlock(), they will still run.
>
> Even worse, holding rtnl_lock() will not stop the bridge multicast layer
> from modifying its br->mdb_list; only br->multicast_lock will.
>
> So you may be better off also acquiring br->multicast_lock, and
> notifying the MDB entries to the switchdev chain _with_the F_DEFER flag.

Like this?

                                      +br_iterator(dsa_cb)
                                      |  lock br->multicask_lock
                                      |  for each mdb
                                      |    br_switchdev_mdb_notify
rtnl_lock                             |      |
 |                                    |      +switchdev_port_obj_._defer
 +switchdev_port_attr_set_deferred    |  unlock br->multicast_lock
   |                                  |
   +dsa_port_mrouter                  |
     |                                |
     +mv88e6xxx_port_mrouter----------+
                                        |
 +--------------------------------------+
 |
rtnl_unlock

(potential task change)

rtnl_lock
 |
 +switchdev_deferred_process
 | flush all queued dfitems in queuing order
 |
rtnl_unlock

I'm not that familiar with the bridge code, but is there any concern 
with potential deadlock here (bewteen rtnl_lock and br->multicast_lock)?

> > Note that on the system I tested, each register read/write takes about 100us
> > to complete. For 100's of mdb groups, this would mean that we will be
> > holding rtnl lock for 10's of ms. I don't know if it's considered too long.
>
> Not sure how this is going to be any better if the iteration over MDB
> entries is done 100% in the driver, though - since its hook,
> dsa_port_mrouter(), runs entirely under rtnl_lock(). >
> Anyway, with the SWITCHDEV_F_DEFER flag, maybe the mdb object
> notifications can be made to run by switchdev only a few at a time, to
> give the network stack time to do other things as well.