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. > 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.
