Date:   Mon, 11 Apr 2022 15:39:08 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Tobias Waldekranz <tobias@...dekranz.com>
Subject: Re: [PATCH v4 net-next 0/3] net: dsa: mv88e6xxx: Implement offload
 of matchall for bridged DSA ports

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

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

Powered by blists - more mailing lists