Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload of matchall for bridged DSA ports

[Mattias Forsblad <mattias.forsblad@gmail.com>]

On 2022-04-11 14:39, Vladimir Oltean wrote:
> On Mon, Apr 11, 2022 at 02:06:30PM +0200, Mattias Forsblad wrote:
>> RFC -> v1: Monitor bridge join/leave and re-evaluate offloading (Vladimir Oltean)
>> v2: Fix code standard compliance (Jakub Kicinski)
>> v3: Fix warning from kernel test robot (<lkp@intel.com>)
>> v4: Check matchall priority (Jakub)
>>     Use boolean type (Vladimir)
>>     Use Vladimirs code for checking foreign interfaces
>>     Drop unused argument (Vladimir)
>>     Add switchdev notifier (Vladimir)
> 
> By switchdev notifier you mean SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV?
> I'm sorry, you must have misunderstood. I said, in reference to
> dp->ds->ops->bridge_local_rcv():
> 
> | Not to mention this should be a cross-chip notifier, maybe a
> | cross-tree notifier.
> 
> https://patchwork.kernel.org/project/netdevbpf/patch/20220404104826.1902292-2-mattias.forsblad@gmail.com/#24805497
> 
> A cross-chip notifier is an event signaled using dsa_tree_notify() and
> handled in switch.c. Its purpose is to replicate an event exactly once
> towards all switches in a multi-switch topology.
> 
> You could have explained that this isn't necessary, because
> dsa_slave_setup_bridge_tc_indr_block(netdev=bridge_dev) indirectly binds
> dsa_slave_setup_bridge_block_cb() which calls dsa_slave_setup_tc_block_cb()
> for each user port under said bridge. So replicating the ds->ops->bridge_local_rcv()
> towards each switch is already taken care of in another way, although
> suboptimally, because if there are 4 user ports under br0 in switch A
> and 4 user ports in switch B, ds->ops->bridge_local_rcv() will be called
> 4 times for switch A and 4 times for switch B. 6 out of those 8 calls
> are for nothing.
> 
> Or you could have said that you don't understand the request and ask me
> to clarify.
> 
> But I don't understand why you've added SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV
> which has no consumer. Initially I thought you'd go back to having the
> bridge monitor flow blocks binding to its ingress chain instead of this
> broken indirect stuff, then emit SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV on
> the switchdev notifier chain which DSA catches and offloads. And initial
> state would be synced/unsynced via attribute replays in
> dsa_port_switchdev_sync_attrs(). At least that would have worked.
> But nope. It really looks like SWITCHDEV_ATTR_ID_BRIDGE_LOCAL_RCV was
> added to appease an unclear comment even if it made no sense.
> 

My thinking was that the notifier I was aware of was the one I implemented
and someway was a preparation for consumers (that didn't exist yes). I didn't even
know about dsa_tree_notify(). So I'll remove that part, yes? Is that ok even
if it's not optimal, like you say?


>>     Only call ops when value have changed (Vladimir)
>>     Add error check (Vladimir)
>>
>> Mattias Forsblad (3):
>>   net: dsa: track whetever bridges have foreign interfaces in them
>>   net: dsa: Add support for offloading tc matchall with drop target
>>   net: dsa: mv88e6xxx: Add HW offload support for tc matchall in Marvell
>>     switches
>>
>>  drivers/net/dsa/mv88e6xxx/chip.c |  17 +-
>>  include/net/dsa.h                |  15 ++
>>  include/net/switchdev.h          |   2 +
>>  net/dsa/dsa2.c                   |   2 +
>>  net/dsa/dsa_priv.h               |   3 +
>>  net/dsa/port.c                   |  14 ++
>>  net/dsa/slave.c                  | 321 +++++++++++++++++++++++++++++--
>>  7 files changed, 361 insertions(+), 13 deletions(-)
>>
>> -- 
>> 2.25.1
>>