[PATCH][net-next] bridge: clean up mtu_set_by_user setting to false and comments

[PATCH][net-next] bridge: clean up mtu_set_by_user setting to false and comments

[Li RongQing]

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

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

Re: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to false and comments

[Nikolay Aleksandrov]

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>

答复: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to false and comments

[Li,Rongqing]

> -----邮件原件-----
> 发件人: 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

Re: 答复: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to false and comments

[Nikolay Aleksandrov]

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
> 

答复: 答复: [PATCH][net-next] bridge: clean up mtu_set_by_user setting to false and comments

[Li,Rongqing]

> 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