| Message ID | 1531464444-26517-1-git-send-email-lirongqing@baidu.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] bridge: clean up mtu_set_by_user setting to false and comments | expand |
On 13/07/18 09:47, Li RongQing wrote: > Once mtu_set_by_user is set to true, br_mtu_auto_adjust will > not run, and no chance to clear mtu_set_by_user. > ^^ This was by design, there is no error here and no "cleanup" is needed. If you read the ndo_change_mtu() call you'll see the comment: /* this flag will be cleared if the MTU was automatically adjusted */ It is the only way we can know that the MTU was automatically adjusted or set by the user manually in which case we need to _stop_ automatically adjusting MTU. This was done to be backwards compatible as much as possible but still give the option to have user-configured MTU which doesn't disappear (and is not overwritten). So please next time read the original commit. From the original commit 804b854d374e ("net: bridge: disable bridge MTU auto tuning if it was set manually"): " ... Let's improve on that situation and allow for the user to set any MTU within ETH_MIN/MAX limits, but once manually configured it is the user's responsibility to keep it correct afterwards. In case the MTU isn't manually set - the behaviour reverts to the previous and the bridge follows the minimum MTU. ... " > and br_mtu_auto_adjust will run only if mtu_set_by_user is > false, so not need to set it to false again > > Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > net/bridge/br_device.c | 1 - > net/bridge/br_if.c | 4 ---- > 2 files changed, 5 deletions(-) > > diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c > index e682a668ce57..c636bc2749c2 100644 > --- a/net/bridge/br_device.c > +++ b/net/bridge/br_device.c > @@ -227,7 +227,6 @@ static int br_change_mtu(struct net_device *dev, int new_mtu) > > dev->mtu = new_mtu; > > - /* this flag will be cleared if the MTU was automatically adjusted */ > br->mtu_set_by_user = true; > #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) > /* remember the MTU in the rtable for PMTU */ > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c > index 05e42d86882d..47c65da4b1be 100644 > --- a/net/bridge/br_if.c > +++ b/net/bridge/br_if.c > @@ -450,11 +450,7 @@ void br_mtu_auto_adjust(struct net_bridge *br) > if (br->mtu_set_by_user) > return; > > - /* change to the minimum MTU and clear the flag which was set by > - * the bridge ndo_change_mtu callback > - */ > dev_set_mtu(br->dev, br_mtu_min(br)); > - br->mtu_set_by_user = false; > } > > static void br_set_gso_limits(struct net_bridge *br) > Nacked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> -----邮件原件----- > 发件人: Nikolay Aleksandrov [mailto:nikolay@cumulusnetworks.com] > 发送时间: 2018年7月13日 16:01 > 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org > 主题: Re: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to > false and comments > > On 13/07/18 09:47, Li RongQing wrote: > > Once mtu_set_by_user is set to true, br_mtu_auto_adjust will not run, > > and no chance to clear mtu_set_by_user. > > > ^^ > This was by design, there is no error here and no "cleanup" is needed. > If you read the ndo_change_mtu() call you'll see the comment: > /* this flag will be cleared if the MTU was automatically adjusted */ > But after this comment, mtu_set_by_user is set to true, and br_mtu_auto_adjust will not truly be run, how to set mtu_set_by_user to false? 230 /* this flag will be cleared if the MTU was automatically adjusted */ 231 br->mtu_set_by_user = true; And the line 457 is useless, since it run only if it is false? 445 void br_mtu_auto_adjust(struct net_bridge *br) 446 { 447 ASSERT_RTNL(); 448 449 /* if the bridge MTU was manually configured don't mess with it */ 450 if (br->mtu_set_by_user) 451 return; 452 453 /* change to the minimum MTU and clear the flag which was set by 454 * the bridge ndo_change_mtu callback 455 */ 456 dev_set_mtu(br->dev, br_mtu_min(br)); 457 br->mtu_set_by_user = false; 458 } -R
On 13/07/18 12:11, Li,Rongqing wrote: > > >> -----邮件原件----- >> 发件人: Nikolay Aleksandrov [mailto:nikolay@cumulusnetworks.com] >> 发送时间: 2018年7月13日 16:01 >> 收件人: Li,Rongqing <lirongqing@baidu.com>; netdev@vger.kernel.org >> 主题: Re: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to >> false and comments >> >> On 13/07/18 09:47, Li RongQing wrote: >>> Once mtu_set_by_user is set to true, br_mtu_auto_adjust will not run, >>> and no chance to clear mtu_set_by_user. >>> >> ^^ >> This was by design, there is no error here and no "cleanup" is needed. >> If you read the ndo_change_mtu() call you'll see the comment: >> /* this flag will be cleared if the MTU was automatically adjusted */ >> > But after this comment, mtu_set_by_user is set to true, and br_mtu_auto_adjust > will not truly be run, how to set mtu_set_by_user to false? It will be run if the mtu_set_by_user is set to "false". > > 230 /* this flag will be cleared if the MTU was automatically adjusted */ > 231 br->mtu_set_by_user = true; > > And the line 457 is useless, since it run only if it is false? dev_set_mtu() calls ndo_change_mtu() which makes mtu_set_by_user = true but that is not really _true_ since this is an automatic MTU adjustment so we need to revert the value to false so the automatic adjustment will continue to work. But if you go ahead and set the MTU yourself manually, then mtu_set_by_user will be equal to true and will stay that way, thus disabling the auto adjust behaviour. This is used to differentiate when auto adjust is used and when user has set the MTU. As I already said everything is working as expected and you should not remove this code. > > 445 void br_mtu_auto_adjust(struct net_bridge *br) > 446 { > 447 ASSERT_RTNL(); > 448 > 449 /* if the bridge MTU was manually configured don't mess with it */ > 450 if (br->mtu_set_by_user) > 451 return; > 452 > 453 /* change to the minimum MTU and clear the flag which was set by > 454 * the bridge ndo_change_mtu callback > 455 */ > 456 dev_set_mtu(br->dev, br_mtu_min(br)); > 457 br->mtu_set_by_user = false; > 458 } > > > -R >
> This is used to differentiate when auto adjust is used and when user has set > the MTU. > As I already said everything is working as expected and you should not > remove this code. > I see, thank you, and sorry for the noise. -R
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index e682a668ce57..c636bc2749c2 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c @@ -227,7 +227,6 @@ static int br_change_mtu(struct net_device *dev, int new_mtu) dev->mtu = new_mtu; - /* this flag will be cleared if the MTU was automatically adjusted */ br->mtu_set_by_user = true; #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER) /* remember the MTU in the rtable for PMTU */ diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c index 05e42d86882d..47c65da4b1be 100644 --- a/net/bridge/br_if.c +++ b/net/bridge/br_if.c @@ -450,11 +450,7 @@ void br_mtu_auto_adjust(struct net_bridge *br) if (br->mtu_set_by_user) return; - /* change to the minimum MTU and clear the flag which was set by - * the bridge ndo_change_mtu callback - */ dev_set_mtu(br->dev, br_mtu_min(br)); - br->mtu_set_by_user = false; } static void br_set_gso_limits(struct net_bridge *br)
Once mtu_set_by_user is set to true, br_mtu_auto_adjust will not run, and no chance to clear mtu_set_by_user. and br_mtu_auto_adjust will run only if mtu_set_by_user is false, so not need to set it to false again Cc: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Signed-off-by: Li RongQing <lirongqing@baidu.com> --- net/bridge/br_device.c | 1 - net/bridge/br_if.c | 4 ---- 2 files changed, 5 deletions(-)