* [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded @ 2021-08-11 13:52 DENG Qingfang 2021-08-11 21:38 ` Ido Schimmel 0 siblings, 1 reply; 7+ messages in thread From: DENG Qingfang @ 2021-08-11 13:52 UTC (permalink / raw) To: Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list Cc: Florian Fainelli, Vladimir Oltean Add BR_ISOLATED flag to BR_PORT_FLAGS_HW_OFFLOAD, to allow switchdev drivers to offload port isolation. Suggested-by: Vladimir Oltean <olteanv@gmail.com> Signed-off-by: DENG Qingfang <dqfext@gmail.com> --- net/bridge/br_switchdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c index 6bf518d78f02..898257153883 100644 --- a/net/bridge/br_switchdev.c +++ b/net/bridge/br_switchdev.c @@ -71,7 +71,8 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, /* Flags that can be offloaded to hardware */ #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ - BR_MCAST_FLOOD | BR_BCAST_FLOOD) + BR_MCAST_FLOOD | BR_BCAST_FLOOD | \ + BR_ISOLATED) int br_switchdev_set_port_flag(struct net_bridge_port *p, unsigned long flags, -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded 2021-08-11 13:52 [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded DENG Qingfang @ 2021-08-11 21:38 ` Ido Schimmel 2021-08-11 21:45 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: Ido Schimmel @ 2021-08-11 21:38 UTC (permalink / raw) To: DENG Qingfang Cc: Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list, Florian Fainelli, Vladimir Oltean On Wed, Aug 11, 2021 at 09:52:46PM +0800, DENG Qingfang wrote: > Add BR_ISOLATED flag to BR_PORT_FLAGS_HW_OFFLOAD, to allow switchdev > drivers to offload port isolation. > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > net/bridge/br_switchdev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > index 6bf518d78f02..898257153883 100644 > --- a/net/bridge/br_switchdev.c > +++ b/net/bridge/br_switchdev.c > @@ -71,7 +71,8 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, > > /* Flags that can be offloaded to hardware */ > #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ > - BR_MCAST_FLOOD | BR_BCAST_FLOOD) > + BR_MCAST_FLOOD | BR_BCAST_FLOOD | \ > + BR_ISOLATED) Why add it now and not as part of a patchset that actually makes use of the flag in a driver that offloads port isolation? > > int br_switchdev_set_port_flag(struct net_bridge_port *p, > unsigned long flags, > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded 2021-08-11 21:38 ` Ido Schimmel @ 2021-08-11 21:45 ` Vladimir Oltean 2021-08-11 21:52 ` Ido Schimmel 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-08-11 21:45 UTC (permalink / raw) To: Ido Schimmel Cc: DENG Qingfang, Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list, Florian Fainelli On Thu, Aug 12, 2021 at 12:38:56AM +0300, Ido Schimmel wrote: > On Wed, Aug 11, 2021 at 09:52:46PM +0800, DENG Qingfang wrote: > > Add BR_ISOLATED flag to BR_PORT_FLAGS_HW_OFFLOAD, to allow switchdev > > drivers to offload port isolation. > > > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > --- > > net/bridge/br_switchdev.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > > index 6bf518d78f02..898257153883 100644 > > --- a/net/bridge/br_switchdev.c > > +++ b/net/bridge/br_switchdev.c > > @@ -71,7 +71,8 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, > > > > /* Flags that can be offloaded to hardware */ > > #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ > > - BR_MCAST_FLOOD | BR_BCAST_FLOOD) > > + BR_MCAST_FLOOD | BR_BCAST_FLOOD | \ > > + BR_ISOLATED) > > Why add it now and not as part of a patchset that actually makes use of > the flag in a driver that offloads port isolation? The way the information got transmitted is a bit unfortunate. Making BR_ISOLATED part of BR_PORT_FLAGS_HW_OFFLOAD is a matter of correctness when switchdev offloads the data path. Since this feature will not work correctly without driver intervention, it makes sense that drivers should reject it currently, which is exactly what this patch accomplishes - it makes the code path go through the SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS driver handlers, which return -EINVAL for everything they don't recognize. (yes, we do still have a problem for drivers that don't catch SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS at all, switchdev will return -EOPNOTSUPP for those which is then ignored, but those are in the minority) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded 2021-08-11 21:45 ` Vladimir Oltean @ 2021-08-11 21:52 ` Ido Schimmel 2021-08-11 21:58 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: Ido Schimmel @ 2021-08-11 21:52 UTC (permalink / raw) To: Vladimir Oltean Cc: DENG Qingfang, Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list, Florian Fainelli On Thu, Aug 12, 2021 at 12:45:06AM +0300, Vladimir Oltean wrote: > On Thu, Aug 12, 2021 at 12:38:56AM +0300, Ido Schimmel wrote: > > On Wed, Aug 11, 2021 at 09:52:46PM +0800, DENG Qingfang wrote: > > > Add BR_ISOLATED flag to BR_PORT_FLAGS_HW_OFFLOAD, to allow switchdev > > > drivers to offload port isolation. > > > > > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > > --- > > > net/bridge/br_switchdev.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > > > index 6bf518d78f02..898257153883 100644 > > > --- a/net/bridge/br_switchdev.c > > > +++ b/net/bridge/br_switchdev.c > > > @@ -71,7 +71,8 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, > > > > > > /* Flags that can be offloaded to hardware */ > > > #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ > > > - BR_MCAST_FLOOD | BR_BCAST_FLOOD) > > > + BR_MCAST_FLOOD | BR_BCAST_FLOOD | \ > > > + BR_ISOLATED) > > > > Why add it now and not as part of a patchset that actually makes use of > > the flag in a driver that offloads port isolation? > > The way the information got transmitted is a bit unfortunate. > > Making BR_ISOLATED part of BR_PORT_FLAGS_HW_OFFLOAD is a matter of > correctness when switchdev offloads the data path. Since this feature > will not work correctly without driver intervention, it makes sense that > drivers should reject it currently, which is exactly what this patch > accomplishes - it makes the code path go through the > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS driver handlers, which return > -EINVAL for everything they don't recognize. If the purpose is correctness, then this is not the only flag that was missed. BR_HAIRPIN_MODE is also relevant for the data path, for example. Anyway, the commit message needs to be reworded to reflect the true purpose of the patch. > > (yes, we do still have a problem for drivers that don't catch > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS at all, switchdev will return > -EOPNOTSUPP for those which is then ignored, but those are in the > minority) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded 2021-08-11 21:52 ` Ido Schimmel @ 2021-08-11 21:58 ` Vladimir Oltean 2021-08-12 6:04 ` DENG Qingfang 0 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-08-11 21:58 UTC (permalink / raw) To: Ido Schimmel Cc: DENG Qingfang, Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list, Florian Fainelli On Thu, Aug 12, 2021 at 12:52:48AM +0300, Ido Schimmel wrote: > On Thu, Aug 12, 2021 at 12:45:06AM +0300, Vladimir Oltean wrote: > > On Thu, Aug 12, 2021 at 12:38:56AM +0300, Ido Schimmel wrote: > > > On Wed, Aug 11, 2021 at 09:52:46PM +0800, DENG Qingfang wrote: > > > > Add BR_ISOLATED flag to BR_PORT_FLAGS_HW_OFFLOAD, to allow switchdev > > > > drivers to offload port isolation. > > > > > > > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > > > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > > > --- > > > > net/bridge/br_switchdev.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c > > > > index 6bf518d78f02..898257153883 100644 > > > > --- a/net/bridge/br_switchdev.c > > > > +++ b/net/bridge/br_switchdev.c > > > > @@ -71,7 +71,8 @@ bool nbp_switchdev_allowed_egress(const struct net_bridge_port *p, > > > > > > > > /* Flags that can be offloaded to hardware */ > > > > #define BR_PORT_FLAGS_HW_OFFLOAD (BR_LEARNING | BR_FLOOD | \ > > > > - BR_MCAST_FLOOD | BR_BCAST_FLOOD) > > > > + BR_MCAST_FLOOD | BR_BCAST_FLOOD | \ > > > > + BR_ISOLATED) > > > > > > Why add it now and not as part of a patchset that actually makes use of > > > the flag in a driver that offloads port isolation? > > > > The way the information got transmitted is a bit unfortunate. > > > > Making BR_ISOLATED part of BR_PORT_FLAGS_HW_OFFLOAD is a matter of > > correctness when switchdev offloads the data path. Since this feature > > will not work correctly without driver intervention, it makes sense that > > drivers should reject it currently, which is exactly what this patch > > accomplishes - it makes the code path go through the > > SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS driver handlers, which return > > -EINVAL for everything they don't recognize. > > If the purpose is correctness, then this is not the only flag that was > missed. BR_HAIRPIN_MODE is also relevant for the data path, for example. I never wanted to suggest that I'm giving a comprehensive answer, I just answered Qingfang's punctual question here: https://lore.kernel.org/netdev/CALW65jbotyW0MSOd-bd1TH_mkiBWhhRCQ29jgn+d12rXdj2pZA@mail.gmail.com/ Tobias also pointed out the same issue about BR_MULTICAST_TO_UNICAST in conjunction with tx_fwd_offload (although the same is probably true even without it): https://patchwork.kernel.org/project/netdevbpf/cover/20210426170411.1789186-1-tobias@waldekranz.com/ > Anyway, the commit message needs to be reworded to reflect the true > purpose of the patch. Agree, and potentially extended with all the bridge port flags which are broken without switchdev driver intervention. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded 2021-08-11 21:58 ` Vladimir Oltean @ 2021-08-12 6:04 ` DENG Qingfang 2021-08-12 10:17 ` Vladimir Oltean 0 siblings, 1 reply; 7+ messages in thread From: DENG Qingfang @ 2021-08-12 6:04 UTC (permalink / raw) To: Vladimir Oltean Cc: Ido Schimmel, Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list, Florian Fainelli On Thu, Aug 12, 2021 at 12:58:33AM +0300, Vladimir Oltean wrote: > On Thu, Aug 12, 2021 at 12:52:48AM +0300, Ido Schimmel wrote: > > > > If the purpose is correctness, then this is not the only flag that was > > missed. BR_HAIRPIN_MODE is also relevant for the data path, for example. > > I never wanted to suggest that I'm giving a comprehensive answer, I just > answered Qingfang's punctual question here: > https://lore.kernel.org/netdev/CALW65jbotyW0MSOd-bd1TH_mkiBWhhRCQ29jgn+d12rXdj2pZA@mail.gmail.com/ > > Tobias also pointed out the same issue about BR_MULTICAST_TO_UNICAST in > conjunction with tx_fwd_offload (although the same is probably true even > without it): > https://patchwork.kernel.org/project/netdevbpf/cover/20210426170411.1789186-1-tobias@waldekranz.com/ > > > Anyway, the commit message needs to be reworded to reflect the true > > purpose of the patch. > > Agree, and potentially extended with all the bridge port flags which are > broken without switchdev driver intervention. So, what else flags should be added to BR_PORT_FLAGS_HW_OFFLOAD? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded 2021-08-12 6:04 ` DENG Qingfang @ 2021-08-12 10:17 ` Vladimir Oltean 0 siblings, 0 replies; 7+ messages in thread From: Vladimir Oltean @ 2021-08-12 10:17 UTC (permalink / raw) To: DENG Qingfang Cc: Ido Schimmel, Roopa Prabhu, Nikolay Aleksandrov, David S. Miller, Jakub Kicinski, moderated list:ETHERNET BRIDGE, open list:ETHERNET BRIDGE, open list, Florian Fainelli On Thu, Aug 12, 2021 at 02:04:10PM +0800, DENG Qingfang wrote: > On Thu, Aug 12, 2021 at 12:58:33AM +0300, Vladimir Oltean wrote: > > On Thu, Aug 12, 2021 at 12:52:48AM +0300, Ido Schimmel wrote: > > > > > > If the purpose is correctness, then this is not the only flag that was > > > missed. BR_HAIRPIN_MODE is also relevant for the data path, for example. > > > > I never wanted to suggest that I'm giving a comprehensive answer, I just > > answered Qingfang's punctual question here: > > https://lore.kernel.org/netdev/CALW65jbotyW0MSOd-bd1TH_mkiBWhhRCQ29jgn+d12rXdj2pZA@mail.gmail.com/ > > > > Tobias also pointed out the same issue about BR_MULTICAST_TO_UNICAST in > > conjunction with tx_fwd_offload (although the same is probably true even > > without it): > > https://patchwork.kernel.org/project/netdevbpf/cover/20210426170411.1789186-1-tobias@waldekranz.com/ > > > > > Anyway, the commit message needs to be reworded to reflect the true > > > purpose of the patch. > > > > Agree, and potentially extended with all the bridge port flags which are > > broken without switchdev driver intervention. > > So, what else flags should be added to BR_PORT_FLAGS_HW_OFFLOAD? I can't think of others beside these 3, BR_ISOLATED, BR_HAIRPIN_MODE and BR_MULTICAST_TO_UNICAST. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-08-12 10:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-11 13:52 [PATCH net-next] net: bridge: switchdev: allow port isolation to be offloaded DENG Qingfang 2021-08-11 21:38 ` Ido Schimmel 2021-08-11 21:45 ` Vladimir Oltean 2021-08-11 21:52 ` Ido Schimmel 2021-08-11 21:58 ` Vladimir Oltean 2021-08-12 6:04 ` DENG Qingfang 2021-08-12 10:17 ` Vladimir Oltean
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).