Date:   Tue, 9 Apr 2019 12:49:10 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Huang Rui <huangruippp@...il.com>, davem@...emloft.net
Cc:     lirongqing@...du.com, netdev@...r.kernel.org,
        Roopa Prabhu <roopa@...ulusnetworks.com>
Subject: Re: [PATCH] net:bridge:bridge mtu auto tuning does not always work

On 09/04/2019 12:20, Nikolay Aleksandrov wrote:
> On 09/04/2019 10:36, Huang Rui wrote:
>> If someone setup a bridge and add a port(for example: eth0)
>> into the bridge, but configure the bridge's mtu which is equal
>> to eth0's mtu, the auto tuning flag will not be set true. But
>> the meaning of the auto tuning flag is that it will be set true
>> if a user configure bridge's mtu.
>>
>> Signed-off-by: Huang Rui <huangruiPPP@...il.com>
>> ---
>>  net/core/dev.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 2b67f2a..ba410d7 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -7670,8 +7670,12 @@ int dev_set_mtu_ext(struct net_device *dev, int new_mtu,
>>  {
>>  	int err, orig_mtu;
>>  
>> -	if (new_mtu == dev->mtu)
>> -		return 0;
>> +	if (new_mtu == dev->mtu) {
>> +		if (dev->priv_flags & IFF_EBRIDGE)
>> +			return __dev_set_mtu(dev, new_mtu);
>> +		else
>> +			return 0;
>> +	}
>>  
>>  	/* MTU must be positive, and in range */
>>  	if (new_mtu < 0 || new_mtu < dev->min_mtu) {
>>
> 
> Nacked-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> 
> No, it is not. If the user specified the MTU then auto-tuning is disabled
> on purpose because that is what the user desired as MTU. We've had so many
> MTU issues over the years, it's hard to get it right and the auto-min/max
> policies work only for some cases, to workaround that we disable the auto
> policy when the user explicitly specifies the MTU[1].
> 
> Also please CC bridge maintainers when sending bridge patches.
> 
> Thanks,
>  Nik
> 
> [1] This has been the behaviour since:
> commit 804b854d374e
> Author: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> Date:   Fri Mar 30 13:46:19 2018 +0300
> 
>     net: bridge: disable bridge MTU auto tuning if it was set manually
>     
>     As Roopa noted today the biggest source of problems when configuring
>     bridge and ports is that the bridge MTU keeps changing automatically on
>     port events (add/del/changemtu). That leads to inconsistent behaviour
>     and network config software needs to chase the MTU and fix it on each
>     such event. 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.
>     
>     Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
>     Signed-off-by: David S. Miller <davem@...emloft.net>
> 

Ahh wait, looking again at the change I get why you're doing that. The commit
message is wrong though, you're trying to disable auto-tuning always even
when setting the same MTU, right ?
Or IOW, I guess by the auto-tuning flag you mean BROPT_MTU_SET_BY_USER ?

I don't like device-specific hacks in the generic code, specifically this
code will not generate an event but will affect state. I don't see a better
approach currently though.

At the very least please add a better explanation and better subject line.
It is not "auto tuning does not always work", but maybe something similar
to "always disable auto-tuning when the user specified MTU" with details
of the case where setting the same MTU doesn't disable auto-tuning currently.

Thanks

Powered by blists - more mailing lists