[PATCH net] bonding: reset the slave's mtu when its be changed

[PATCH net] bonding: reset the slave's mtu when its be changed

[Ding Tianhong]

All slave should have the same mtu with mastet's, and the bond do it when
enslave the slave, but the user could change the slave's mtu, it will cause
the master and slave have different mtu, althrough in AB mode, it does not
matter if the slave is not the current slave, but in other mode, it is incorrect,
so reset the slave's mtu like the master set.

Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/net/bonding/bond_main.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 398e299..e7b5bcf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,
 		 */
 		break;
 	case NETDEV_CHANGEMTU:
-		/*
-		 * TODO: Should slaves be allowed to
-		 * independently alter their MTU?  For
-		 * an active-backup bond, slaves need
-		 * not be the same type of device, so
-		 * MTUs may vary.  For other modes,
-		 * slaves arguably should have the
-		 * same MTUs. To do this, we'd need to
-		 * take over the slave's change_mtu
-		 * function for the duration of their
-		 * servitude.
+		/* All slave should have the same mtu
+		 * as master.
 		 */
+		if (slave->dev->mtu != bond->dev->mtu) {
+			int res;
+			slave->original_mtu = slave->dev->mtu;
+			res = dev_set_mtu(slave->dev, bond->dev->mtu);
+			if (res)
+				pr_debug("Error %d calling dev_set_mtu for slave %s\n",
+					 res, slave->dev->name);
+		}
 		break;
 	case NETDEV_CHANGENAME:
 		/*
--
1.8.0

Re: [PATCH net] bonding: reset the slave's mtu when its be changed

[Veaceslav Falico]

On Fri, Jan 10, 2014 at 07:32:51PM +0800, Ding Tianhong wrote:
>All slave should have the same mtu with mastet's, and the bond do it when
>enslave the slave, but the user could change the slave's mtu, it will cause
>the master and slave have different mtu, althrough in AB mode, it does not
>matter if the slave is not the current slave, but in other mode, it is incorrect,
>so reset the slave's mtu like the master set.

Why "net"? It's not a bugfix, it's a feature, and really discussable.

Also, wrt the actual change - why do you think it's incorrect for slaves in
bonding mode other than AB to have different MTU values? I don't see any
reason for it, from the top of the head.

>
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 398e299..e7b5bcf 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,
> 		 */
> 		break;
> 	case NETDEV_CHANGEMTU:
>-		/*
>-		 * TODO: Should slaves be allowed to
>-		 * independently alter their MTU?  For
>-		 * an active-backup bond, slaves need
>-		 * not be the same type of device, so
>-		 * MTUs may vary.  For other modes,
>-		 * slaves arguably should have the
>-		 * same MTUs. To do this, we'd need to
>-		 * take over the slave's change_mtu
>-		 * function for the duration of their
>-		 * servitude.
>+		/* All slave should have the same mtu
>+		 * as master.
> 		 */
>+		if (slave->dev->mtu != bond->dev->mtu) {

If we've got the event then it means it was changed to something different.
No need to verify.

>+			int res;
>+			slave->original_mtu = slave->dev->mtu;

If we're refusing to apply the *new* mtu, then why should we save it as the
original? The original_mtu is the mtu that the slave had before it was
enslaved.

>+			res = dev_set_mtu(slave->dev, bond->dev->mtu);
>+			if (res)
>+				pr_debug("Error %d calling dev_set_mtu for slave %s\n",
>+					 res, slave->dev->name);
>+		}

Also, bonding should be vocal about changing forcibly the mtu - otherwise
we'd end up with silently dropping the changes:

ifconfig eth0 mtu 9000
echo $?
-> 0
ifconfig eth0
MTU: 1500

or something like that, it will pass it up, refusing changes:

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06c445..0b36045 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2846,19 +2846,8 @@ static int bond_slave_netdev_event(unsigned long event,
  		 */
  		break;
  	case NETDEV_CHANGEMTU:
-		/*
-		 * TODO: Should slaves be allowed to
-		 * independently alter their MTU?  For
-		 * an active-backup bond, slaves need
-		 * not be the same type of device, so
-		 * MTUs may vary.  For other modes,
-		 * slaves arguably should have the
-		 * same MTUs. To do this, we'd need to
-		 * take over the slave's change_mtu
-		 * function for the duration of their
-		 * servitude.
-		 */
-		break;
+		/* don't permit slaves to change their MTU */
+		return NOTIFY_BAD;
  	case NETDEV_CHANGENAME:
  		/*
  		 * TODO: handle changing the primary's name

> 		break;
> 	case NETDEV_CHANGENAME:
> 		/*
>--
>1.8.0
>
>

Re: [PATCH net] bonding: reset the slave's mtu when its be changed

[Ding Tianhong]

On 2014/1/10 20:19, Veaceslav Falico wrote:
> On Fri, Jan 10, 2014 at 07:32:51PM +0800, Ding Tianhong wrote:
>> All slave should have the same mtu with mastet's, and the bond do it when
>> enslave the slave, but the user could change the slave's mtu, it will cause
>> the master and slave have different mtu, althrough in AB mode, it does not
>> matter if the slave is not the current slave, but in other mode, it is incorrect,
>> so reset the slave's mtu like the master set.
> 
> Why "net"? It's not a bugfix, it's a feature, and really discussable.
> 
> Also, wrt the actual change - why do you think it's incorrect for slaves in
> bonding mode other than AB to have different MTU values? I don't see any
> reason for it, from the top of the head.
> 

Ok, I will test more situation for every mode when slave's mtu changed, I am not sure
what will happened yet, if some links was interrupt, I thinks it is a bug. 

>>
>> Cc: Jay Vosburgh <fubar@us.ibm.com>
>> Cc: Veaceslav Falico <vfalico@redhat.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 398e299..e7b5bcf 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,
>>          */
>>         break;
>>     case NETDEV_CHANGEMTU:
>> -        /*
>> -         * TODO: Should slaves be allowed to
>> -         * independently alter their MTU?  For
>> -         * an active-backup bond, slaves need
>> -         * not be the same type of device, so
>> -         * MTUs may vary.  For other modes,
>> -         * slaves arguably should have the
>> -         * same MTUs. To do this, we'd need to
>> -         * take over the slave's change_mtu
>> -         * function for the duration of their
>> -         * servitude.
>> +        /* All slave should have the same mtu
>> +         * as master.
>>          */
>> +        if (slave->dev->mtu != bond->dev->mtu) {
> 
> If we've got the event then it means it was changed to something different.
> No need to verify.
> 
>> +            int res;
>> +            slave->original_mtu = slave->dev->mtu;
> 
> If we're refusing to apply the *new* mtu, then why should we save it as the
> original? The original_mtu is the mtu that the slave had before it was
> enslaved.
> 
the bond always save the slave's old mtu and set new one, so I did it again,
pls miss it, I think we should forbidden to change the mtu.


>> +            res = dev_set_mtu(slave->dev, bond->dev->mtu);
>> +            if (res)
>> +                pr_debug("Error %d calling dev_set_mtu for slave %s\n",
>> +                     res, slave->dev->name);
>> +        }
> 
> Also, bonding should be vocal about changing forcibly the mtu - otherwise
> we'd end up with silently dropping the changes:
> 
> ifconfig eth0 mtu 9000
> echo $?
> -> 0
> ifconfig eth0
> MTU: 1500
> 
> or something like that, it will pass it up, refusing changes:
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e06c445..0b36045 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2846,19 +2846,8 @@ static int bond_slave_netdev_event(unsigned long event,
>           */
>          break;
>      case NETDEV_CHANGEMTU:
> -        /*
> -         * TODO: Should slaves be allowed to
> -         * independently alter their MTU?  For
> -         * an active-backup bond, slaves need
> -         * not be the same type of device, so
> -         * MTUs may vary.  For other modes,
> -         * slaves arguably should have the
> -         * same MTUs. To do this, we'd need to
> -         * take over the slave's change_mtu
> -         * function for the duration of their
> -         * servitude.
> -         */
> -        break;
> +        /* don't permit slaves to change their MTU */
> +        return NOTIFY_BAD;
>      case NETDEV_CHANGENAME:
>          /*
>           * TODO: handle changing the primary's name
> 
>>         break;
>>     case NETDEV_CHANGENAME:
>>         /*
>> -- 
>> 1.8.0
>>

Yes, no problem.

Regards
Ding

>>
> 
> .
> 

Re: [PATCH net] bonding: reset the slave's mtu when its be changed

[Ding Tianhong]

On 2014/1/12 13:18, Ding Tianhong wrote:
> On 2014/1/10 20:19, Veaceslav Falico wrote:
>> On Fri, Jan 10, 2014 at 07:32:51PM +0800, Ding Tianhong wrote:
>>> All slave should have the same mtu with mastet's, and the bond do it when
>>> enslave the slave, but the user could change the slave's mtu, it will cause
>>> the master and slave have different mtu, althrough in AB mode, it does not
>>> matter if the slave is not the current slave, but in other mode, it is incorrect,
>>> so reset the slave's mtu like the master set.
>>
>> Why "net"? It's not a bugfix, it's a feature, and really discussable.
>>
>> Also, wrt the actual change - why do you think it's incorrect for slaves in
>> bonding mode other than AB to have different MTU values? I don't see any
>> reason for it, from the top of the head.
>>
> 
> Ok, I will test more situation for every mode when slave's mtu changed, I am not sure
> what will happened yet, if some links was interrupt, I thinks it is a bug. 
> 
>>>

I have test several mode for bonding when the slave mtu changed:

RR(0)	0<mtu<1500 		ok
AB(1)	0<mtu<1500		loss packets
XOR(2)	0<mtu<1500		ok
Broadcast(3)	0<mtu<1500	ok
LACP		0<mtu<1500	loss packets


so I think we should not let the mtu set for slave.

Re: [PATCH net] bonding: reset the slave's mtu when its be changed

[Veaceslav Falico]

On Tue, Jan 14, 2014 at 10:11:45AM +0800, Ding Tianhong wrote:
>On 2014/1/12 13:18, Ding Tianhong wrote:
>> On 2014/1/10 20:19, Veaceslav Falico wrote:
>>> On Fri, Jan 10, 2014 at 07:32:51PM +0800, Ding Tianhong wrote:
>>>> All slave should have the same mtu with mastet's, and the bond do it when
>>>> enslave the slave, but the user could change the slave's mtu, it will cause
>>>> the master and slave have different mtu, althrough in AB mode, it does not
>>>> matter if the slave is not the current slave, but in other mode, it is incorrect,
>>>> so reset the slave's mtu like the master set.
>>>
>>> Why "net"? It's not a bugfix, it's a feature, and really discussable.
>>>
>>> Also, wrt the actual change - why do you think it's incorrect for slaves in
>>> bonding mode other than AB to have different MTU values? I don't see any
>>> reason for it, from the top of the head.
>>>
>>
>> Ok, I will test more situation for every mode when slave's mtu changed, I am not sure
>> what will happened yet, if some links was interrupt, I thinks it is a bug.
>>
>>>>
>
>I have test several mode for bonding when the slave mtu changed:
>
>RR(0)	0<mtu<1500 		ok
>AB(1)	0<mtu<1500		loss packets
>XOR(2)	0<mtu<1500		ok
>Broadcast(3)	0<mtu<1500	ok
>LACP		0<mtu<1500	loss packets
>
>
>so I think we should not let the mtu set for slave.

Why do you see lost packets?

>

Re: [PATCH net] bonding: reset the slave's mtu when its be changed

[David Miller]

From: Ding Tianhong <dingtianhong@huawei.com>
Date: Fri, 10 Jan 2014 19:32:51 +0800

> All slave should have the same mtu with mastet's, and the bond do it when
                                          ^^^^^^

Typo, you mean "master"

Re: [PATCH net] bonding: reset the slave's mtu when its be changed

[Sergei Shtylyov]

Hello.

On 01/10/2014 02:32 PM, Ding Tianhong wrote:

> All slave should have the same mtu with mastet's, and the bond do it when

    Only "master", already noted by Dave.

> enslave the slave, but the user could change the slave's mtu, it will cause
> the master and slave have different mtu, althrough in AB mode, it does not

    Only "although".

> matter if the slave is not the current slave, but in other mode, it is incorrect,
> so reset the slave's mtu like the master set.

> Cc: Jay Vosburgh <fubar@us.ibm.com>
> Cc: Veaceslav Falico <vfalico@redhat.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>   drivers/net/bonding/bond_main.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 398e299..e7b5bcf 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,
>   		 */
>   		break;
>   	case NETDEV_CHANGEMTU:
> -		/*
> -		 * TODO: Should slaves be allowed to
> -		 * independently alter their MTU?  For
> -		 * an active-backup bond, slaves need
> -		 * not be the same type of device, so
> -		 * MTUs may vary.  For other modes,
> -		 * slaves arguably should have the
> -		 * same MTUs. To do this, we'd need to
> -		 * take over the slave's change_mtu
> -		 * function for the duration of their
> -		 * servitude.
> +		/* All slave should have the same mtu
> +		 * as master.
>   		 */
> +		if (slave->dev->mtu != bond->dev->mtu) {
> +			int res;

    Please insert empty line after declaration.

> +			slave->original_mtu = slave->dev->mtu;
> +			res = dev_set_mtu(slave->dev, bond->dev->mtu);
> +			if (res)
> +				pr_debug("Error %d calling dev_set_mtu for slave %s\n",
> +					 res, slave->dev->name);

    {} wouldn't hurt around multi-line *if* arm.

> +		}
>   		break;
>   	case NETDEV_CHANGENAME:
>   		/*

WBR, Sergei