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

login
register
mail settings
Submitter Veaceslav Falico
Date Jan. 10, 2014, 12:19 p.m.
Message ID <20140110121932.GC4132@redhat.com>
Download mbox | patch
Permalink /patch/309258/
State Not Applicable
Headers show

Comments

Veaceslav Falico - Jan. 10, 2014, 12:19 p.m.
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:


> 		break;
> 	case NETDEV_CHANGENAME:
> 		/*
>--
>1.8.0
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
dingtianhong - Jan. 12, 2014, 5:18 a.m.
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

>>
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
dingtianhong - Jan. 14, 2014, 2:11 a.m.
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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Veaceslav Falico - Jan. 14, 2014, 6:03 a.m.
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?

>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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