[PATCH net] net: dsa: apply MTU normalization for ports that join a LAG under a bridge too

[PATCH net] net: dsa: apply MTU normalization for ports that join a LAG under a bridge too

[Vladimir Oltean]

We want the MTU normalization logic to apply each time
dsa_port_bridge_join is called, so instead of chasing all callers of
that function, we should move the call within the bridge_join function
itself.

Fixes: 185c9a760a61 ("net: dsa: call dsa_port_bridge_join when joining a LAG that is already in a bridge")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h | 1 +
 net/dsa/port.c     | 2 ++
 net/dsa/slave.c    | 4 +---
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 6988066d937f..6e01e796920f 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -325,6 +325,7 @@ int dsa_slave_register_notifier(void);
 void dsa_slave_unregister_notifier(void);
 void dsa_slave_setup_tagger(struct net_device *slave);
 int dsa_slave_change_mtu(struct net_device *dev, int new_mtu);
+void dsa_bridge_mtu_normalization(struct dsa_port *dp);
 
 static inline struct dsa_port *dsa_slave_to_port(const struct net_device *dev)
 {
diff --git a/net/dsa/port.c b/net/dsa/port.c
index b6208eecdf4b..eedd9881e1ba 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -386,6 +386,8 @@ int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
 	if (err)
 		goto out_rollback_unoffload;
 
+	dsa_bridge_mtu_normalization(dp);
+
 	return 0;
 
 out_rollback_unoffload:
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index b08b2c70702d..124e01ca3312 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1456,7 +1456,7 @@ static void dsa_hw_port_list_free(struct list_head *hw_port_list)
 }
 
 /* Make the hardware datapath to/from @dev limited to a common MTU */
-static void dsa_bridge_mtu_normalization(struct dsa_port *dp)
+void dsa_bridge_mtu_normalization(struct dsa_port *dp)
 {
 	struct list_head hw_port_list;
 	struct dsa_switch_tree *dst;
@@ -2013,8 +2013,6 @@ static int dsa_slave_changeupper(struct net_device *dev,
 	if (netif_is_bridge_master(info->upper_dev)) {
 		if (info->linking) {
 			err = dsa_port_bridge_join(dp, info->upper_dev, extack);
-			if (!err)
-				dsa_bridge_mtu_normalization(dp);
 			err = notifier_from_errno(err);
 		} else {
 			dsa_port_bridge_leave(dp, info->upper_dev);
-- 
2.25.1

Re: [PATCH net] net: dsa: apply MTU normalization for ports that join a LAG under a bridge too

[Vladimir Oltean]

On Wed, Aug 11, 2021 at 03:45:20PM +0300, Vladimir Oltean wrote:
> We want the MTU normalization logic to apply each time
> dsa_port_bridge_join is called, so instead of chasing all callers of
> that function, we should move the call within the bridge_join function
> itself.
> 
> Fixes: 185c9a760a61 ("net: dsa: call dsa_port_bridge_join when joining a LAG that is already in a bridge")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

I forgot to rebase this patch on top of 'net' and now I notice that it
conflicts with the switchdev_bridge_port_offload() work.

Do we feel that the issue this patch fixes isn't too big and the patch
can go into net-next, to avoid conflicts and all that?

Re: [PATCH net] net: dsa: apply MTU normalization for ports that join a LAG under a bridge too

[Jakub Kicinski]

On Wed, 11 Aug 2021 19:44:41 +0300 Vladimir Oltean wrote:
> On Wed, Aug 11, 2021 at 03:45:20PM +0300, Vladimir Oltean wrote:
> > We want the MTU normalization logic to apply each time
> > dsa_port_bridge_join is called, so instead of chasing all callers of
> > that function, we should move the call within the bridge_join function
> > itself.
> > 
> > Fixes: 185c9a760a61 ("net: dsa: call dsa_port_bridge_join when joining a LAG that is already in a bridge")
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---  
> 
> I forgot to rebase this patch on top of 'net' and now I notice that it
> conflicts with the switchdev_bridge_port_offload() work.
> 
> Do we feel that the issue this patch fixes isn't too big and the patch
> can go into net-next, to avoid conflicts and all that?

The commit message doesn't really describe the impact so hard to judge,
but either way you want to go - we'll need a repost so it can be build
tested.

Conflicts are not a huge deal. Obviously always best to wait for trees
to merge if that fixes things, but if net has dependency on net-next
you should just describe what you want the resolution to look like we
should be able to execute :)

Re: [PATCH net] net: dsa: apply MTU normalization for ports that join a LAG under a bridge too

[Vladimir Oltean]

On Wed, Aug 11, 2021 at 03:38:33PM -0700, Jakub Kicinski wrote:
> On Wed, 11 Aug 2021 19:44:41 +0300 Vladimir Oltean wrote:
> > On Wed, Aug 11, 2021 at 03:45:20PM +0300, Vladimir Oltean wrote:
> > > We want the MTU normalization logic to apply each time
> > > dsa_port_bridge_join is called, so instead of chasing all callers of
> > > that function, we should move the call within the bridge_join function
> > > itself.
> > > 
> > > Fixes: 185c9a760a61 ("net: dsa: call dsa_port_bridge_join when joining a LAG that is already in a bridge")
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---  
> > 
> > I forgot to rebase this patch on top of 'net' and now I notice that it
> > conflicts with the switchdev_bridge_port_offload() work.
> > 
> > Do we feel that the issue this patch fixes isn't too big and the patch
> > can go into net-next, to avoid conflicts and all that?
> 
> The commit message doesn't really describe the impact so hard to judge,
> but either way you want to go - we'll need a repost so it can be build
> tested.

The impact is that if a DSA interface joins a bonding/team that is in a
bridge and the bonding/team had an MTU of 9000 bytes, the DSA interface
will still have what it had before (probably 1500). I found this through
code inspection, didn't hit an actual bug or anything like that. It
seems fairly low impact to me, and a rare occurrence in any case. The
common path is for the DSA interface to first join the bond, then the
bond to join the bridge anyway, and that would work I think.

Later edit: I just realized, while typing, that it's going to be more
complicated than that, so I'm going to just drop the patch at least for
now. While bond_enslave() does in fact make the lower interface inherit
the bond's MTU, that's not what we want here. DSA switches want their
bridged ports to have the same MTU, and they change it dynamically
whenever one MTU changes, but they don't change the MTU of any upper
interfaces they might have. So with the normalization logic as applied
by this patch, ports that join a bond will have an MTU of 9000, but the
bond itself will still have 1500, since
(a) DSA doesn't change it
(b) the bonding driver has a NETDEV_CHANGEMTU handler implementation
    which just wonders whether it should let DSA lowers change their MTU
    in the first place or not.

> Conflicts are not a huge deal. Obviously always best to wait for trees
> to merge if that fixes things, but if net has dependency on net-next
> you should just describe what you want the resolution to look like we
> should be able to execute :)

Yeah, thanks, I noticed you duly applied the instructions in the last
conflicting patch I sent exactly as described, and I appreciate it.
But I don't like conflicts either way, they're a pain to deal with when
you're backporting patches, since you can never get a clean cherry pick
list, so I try to avoid them myself now if I can.