[PATCH] net:bridge:bridge mtu auto tuning does not always work

[PATCH] net:bridge:bridge mtu auto tuning does not always work

[Huang Rui]

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@gmail.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) {
-- 
1.8.3.1

Re: [PATCH] net:bridge:bridge mtu auto tuning does not always work

[Nikolay Aleksandrov]

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@gmail.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@cumulusnetworks.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@cumulusnetworks.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@cumulusnetworks.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] net:bridge:bridge mtu auto tuning does not always work

[Nikolay Aleksandrov]

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@gmail.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@cumulusnetworks.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@cumulusnetworks.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@cumulusnetworks.com>
>     Signed-off-by: David S. Miller <davem@davemloft.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

Re: [PATCH] net:bridge:bridge mtu auto tuning does not always work

[Nikolay Aleksandrov]

On 09/04/2019 12:49, Nikolay Aleksandrov wrote:
> 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
[snip]
> 
> 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
> 

One more thing - this has the potential to break setups because a lot of
the network configuration software always set the device MTU and will
disable the auto-tuning by default with this change.
IIRC that was the reason I dropped it from my original change.