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

[Joseph Huang <joseph.huang.2024@gmail.com>]

On 4/5/2024 2:58 PM, Joseph Huang wrote:
> Hi Vladimir,
> 
> On 4/5/2024 7:07 AM, Vladimir Oltean wrote:
>> On Thu, Apr 04, 2024 at 04:43:38PM -0400, Joseph Huang wrote:
>>> Hi Vladimir,
>>>
>>> On 4/2/2024 8:23 AM, Vladimir Oltean wrote:
>>>> Can you comment on the feasibility/infeasibility of Tobias' proposal 
>>>> of:
>>>> "The bridge could just provide some MDB iterator to save us from having
>>>> to cache all the configured groups."?
>>>> https://lore.kernel.org/netdev/87sg31n04a.fsf@waldekranz.com/
>>>>
>>>> What is done here will have to be scaled to many drivers - potentially
>>>> all existing DSA ones, as far as I'm aware.
>>>>
>>>
>>> I thought about implementing an MDB iterator as suggested by Tobias, 
>>> but I'm
>>> a bit concerned about the coherence of these MDB objects. In theory, 
>>> when
>>> the device driver is trying to act on an event, the source of the 
>>> trigger
>>> may have changed its state in the bridge already.
>>
>> Yes, this is the result of SWITCHDEV_F_DEFER, used by both
>> SWITCHDEV_ATTR_ID_PORT_MROUTER and SWITCHDEV_OBJ_ID_PORT_MDB.
>>
>>> If, upon receiving an event in the device driver, we iterate over what
>>> the bridge has at that instant, the differences between the worlds as
>>> seen by the bridge and the device driver might lead to some unexpected
>>> results.
>>
>> Translated: iterating over bridge MDB objects needs to be serialized
>> with new switchdev events by acquiring rtnl_lock(). Then, once switchdev
>> events are temporarily blocked, the pending ones need to be flushed
>> using switchdev_deferred_process(), so resync the bridge state with the
>> driver state. Once the resync is done, the iteration is safe until
>> rtnl_unlock().
>>
>> Applied to our case, the MDB iterator is needed in 
>> mv88e6xxx_port_mrouter().
>> This is already called with rtnl_lock() acquired. The resync procedure
>> will indirectly call mv88e6xxx_port_mdb_add()/mv88e6xxx_port_mdb_del()
>> through switchdev_deferred_process(), and then the walk is consistent
>> for the remainder of the mv88e6xxx_port_mrouter() function.
>>
>> A helper which does this is what would be required - an iterator
>> function which calls an int (*cb)(struct net_device *brport, const 
>> struct switchdev_obj_port_mdb *mdb)
>> for each MDB entry. The DSA core could then offer some post-processing
>> services over this API, to recover the struct dsa_port associated with
>> the bridge port (in the LAG case they aren't the same) and the address
>> database associated with the bridge.

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

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.


>>
>> Do you think there would be unexpected results even if we did this?
>> br_switchdev_mdb_replay() needs to handle a similarly complicated
>> situation of synchronizing with deferred MDB events.
>>  >> However, if we cache the MDB objects in the device driver, at least
>>> the order in which the events took place will be coherent and at any
>>> give time the state of the MDB objects in the device driver can be
>>> guaranteed to be sane. This is also the approach the prestera device
>>> driver took.
>>
>> Not contesting this, but I wouldn't like to see MDBs cached in each
>> device driver just for this. Switchdev is not very high on the list of
>> APIs which are easy to use, and making MDB caching a requirement
>> (for the common case that MDB entry destinations need software fixups
>> with the mrouter ports) isn't exactly going to make that any better.
>> Others' opinion may differ, but mine is that core offload APIs need to
>> consider what hardware is available in the real world, make the common
>> case easy, and the advanced cases possible. Rather than make every case
>> "advanced" :)
> 
> Just throwing some random ideas out there. Do you think it would make 
> more sense if this whole solution (rtnl_lock, iterator cb,...etc.) is 
> moved up to DSA so that other DSA drivers could benefit from it? I 
> thought about implement it (not the iterator, the current form) in DSA 
> at first, but I'm not sure how other drivers would behave, so I did it 
> with mv instead.
> 
> I guess the question is, is the current limitation (mrouter not properly 
> offloaded) an issue specific to mv or is it a limitation of hardware 
> offloading in general? I tend to think it's the latter.
> 
> But then again, if we move it to DSA, we would lose the benefit of the 
> optimization of consolidating multiple register writes into just one (as 
> done in patch 10 currently), unless we add a new switch op which takes a 
> portvec instead of a port when modifying mdb's.