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

[Nikolay Aleksandrov <nikolay@cumulusnetworks.com>]

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>