[patch net-next] bonding: allow to add vlans on top of empty bond

[patch net-next] bonding: allow to add vlans on top of empty bond

[Jiri Pirko]

This limitation maybe had some reason in the past, but now there is not
one -> removing this.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 drivers/net/bonding/bond_main.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 3a451b6..ffefb70 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1001,12 +1001,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
 	netdev_features_t mask;
 	struct slave *slave;
 
-	if (!bond_has_slaves(bond)) {
-		/* Disable adding VLANs to empty bond. But why? --mq */
-		features |= NETIF_F_VLAN_CHALLENGED;
-		return features;
-	}
-
 	mask = features;
 	features &= ~NETIF_F_ONE_FOR_ALL;
 	features |= NETIF_F_ALL_FOR_ALL;
@@ -3956,13 +3950,6 @@ void bond_setup(struct net_device *bond_dev)
 	bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT;
 	bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
 
-	/* At first, we block adding VLANs. That's the only way to
-	 * prevent problems that occur when adding VLANs over an
-	 * empty bond. The block will be removed once non-challenged
-	 * slaves are enslaved.
-	 */
-	bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
-
 	/* don't acquire bond device's netif_tx_lock when
 	 * transmitting */
 	bond_dev->features |= NETIF_F_LLTX;
-- 
1.9.0

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Tom Gundersen]

Hi Jiri,

On Fri, Jun 27, 2014 at 4:13 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> This limitation maybe had some reason in the past, but now there is not
> one -> removing this.

If this restriction is really no longer needed, it would be very
helpful if we could drop it. See for instnace
<https://bugs.freedesktop.org/show_bug.cgi?id=76077#c5>.

Cheers,

Tom

> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>  drivers/net/bonding/bond_main.c | 13 -------------
>  1 file changed, 13 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 3a451b6..ffefb70 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1001,12 +1001,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>         netdev_features_t mask;
>         struct slave *slave;
>
> -       if (!bond_has_slaves(bond)) {
> -               /* Disable adding VLANs to empty bond. But why? --mq */
> -               features |= NETIF_F_VLAN_CHALLENGED;
> -               return features;
> -       }
> -
>         mask = features;
>         features &= ~NETIF_F_ONE_FOR_ALL;
>         features |= NETIF_F_ALL_FOR_ALL;
> @@ -3956,13 +3950,6 @@ void bond_setup(struct net_device *bond_dev)
>         bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT;
>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>
> -       /* At first, we block adding VLANs. That's the only way to
> -        * prevent problems that occur when adding VLANs over an
> -        * empty bond. The block will be removed once non-challenged
> -        * slaves are enslaved.
> -        */
> -       bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
> -
>         /* don't acquire bond device's netif_tx_lock when
>          * transmitting */
>         bond_dev->features |= NETIF_F_LLTX;
> --
> 1.9.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

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Jay Vosburgh]

Tom Gundersen <teg@jklm.no> wrote:

>Hi Jiri,
>
>On Fri, Jun 27, 2014 at 4:13 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> This limitation maybe had some reason in the past, but now there is not
>> one -> removing this.
>
>If this restriction is really no longer needed, it would be very
>helpful if we could drop it. See for instnace
><https://bugs.freedesktop.org/show_bug.cgi?id=76077#c5>.

	The reason for the restriction is that, in the past, the bond
did not have a MAC address until the first slave was added, so adding a
VLAN wouldn't work properly.

	Currently, a random MAC is assigned to the bond when there are
no slaves, so that problem is no longer an issue (as presumably the VLAN
can handle the MAC change when the first slave goes in to the bond).

	While you're at it, you might want to also check and possibly
remove some of the MAC-related warnings related to removing the last
slave, e.g.,

        if (!bond_has_slaves(bond)) {
                bond_set_carrier(bond);
                eth_hw_addr_random(bond_dev);

                if (vlan_uses_dev(bond_dev)) {
                        pr_warn("%s: Warning: clearing HW address of %s while it still has VLANs\n",
                                bond_dev->name, bond_dev->name);
                        pr_warn("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs\n",
                                bond_dev->name);
                }
        }

	This warning may not be useful any longer, since the MAC should
update correctly without user action when re-adding the first slave.

	-J


>Cheers,
>
>Tom
>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>  drivers/net/bonding/bond_main.c | 13 -------------
>>  1 file changed, 13 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 3a451b6..ffefb70 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1001,12 +1001,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>>         netdev_features_t mask;
>>         struct slave *slave;
>>
>> -       if (!bond_has_slaves(bond)) {
>> -               /* Disable adding VLANs to empty bond. But why? --mq */
>> -               features |= NETIF_F_VLAN_CHALLENGED;
>> -               return features;
>> -       }
>> -
>>         mask = features;
>>         features &= ~NETIF_F_ONE_FOR_ALL;
>>         features |= NETIF_F_ALL_FOR_ALL;
>> @@ -3956,13 +3950,6 @@ void bond_setup(struct net_device *bond_dev)
>>         bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT;
>>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>>
>> -       /* At first, we block adding VLANs. That's the only way to
>> -        * prevent problems that occur when adding VLANs over an
>> -        * empty bond. The block will be removed once non-challenged
>> -        * slaves are enslaved.
>> -        */
>> -       bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
>> -
>>         /* don't acquire bond device's netif_tx_lock when
>>          * transmitting */
>>         bond_dev->features |= NETIF_F_LLTX;
>> --
>> 1.9.0

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Veaceslav Falico]

On Fri, Jun 27, 2014 at 04:13:12PM +0200, Jiri Pirko wrote:
>This limitation maybe had some reason in the past, but now there is not
>one -> removing this.

Yeah, purely legacy stuff.

>
>Signed-off-by: Jiri Pirko <jiri@resnulli.us>

For the patch:

Acked-by: Veaceslav Falico <vfalico@gmail.com>

It would be also nice if you could have time to follow up with Jay's
remarks :).

>---
> drivers/net/bonding/bond_main.c | 13 -------------
> 1 file changed, 13 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 3a451b6..ffefb70 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1001,12 +1001,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
> 	netdev_features_t mask;
> 	struct slave *slave;
>
>-	if (!bond_has_slaves(bond)) {
>-		/* Disable adding VLANs to empty bond. But why? --mq */
>-		features |= NETIF_F_VLAN_CHALLENGED;
>-		return features;
>-	}
>-
> 	mask = features;
> 	features &= ~NETIF_F_ONE_FOR_ALL;
> 	features |= NETIF_F_ALL_FOR_ALL;
>@@ -3956,13 +3950,6 @@ void bond_setup(struct net_device *bond_dev)
> 	bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT;
> 	bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>
>-	/* At first, we block adding VLANs. That's the only way to
>-	 * prevent problems that occur when adding VLANs over an
>-	 * empty bond. The block will be removed once non-challenged
>-	 * slaves are enslaved.
>-	 */
>-	bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
>-
> 	/* don't acquire bond device's netif_tx_lock when
> 	 * transmitting */
> 	bond_dev->features |= NETIF_F_LLTX;
>-- 
>1.9.0
>

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Jiri Pirko]

Sat, Jun 28, 2014 at 09:30:15AM CEST, vfalico@gmail.com wrote:
>On Fri, Jun 27, 2014 at 04:13:12PM +0200, Jiri Pirko wrote:
>>This limitation maybe had some reason in the past, but now there is not
>>one -> removing this.
>
>Yeah, purely legacy stuff.
>
>>
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>
>For the patch:
>
>Acked-by: Veaceslav Falico <vfalico@gmail.com>
>
>It would be also nice if you could have time to follow up with Jay's
>remarks :).

Allright. Will prepare follow-up as well.

>
>>---
>>drivers/net/bonding/bond_main.c | 13 -------------
>>1 file changed, 13 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 3a451b6..ffefb70 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1001,12 +1001,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>>	netdev_features_t mask;
>>	struct slave *slave;
>>
>>-	if (!bond_has_slaves(bond)) {
>>-		/* Disable adding VLANs to empty bond. But why? --mq */
>>-		features |= NETIF_F_VLAN_CHALLENGED;
>>-		return features;
>>-	}
>>-
>>	mask = features;
>>	features &= ~NETIF_F_ONE_FOR_ALL;
>>	features |= NETIF_F_ALL_FOR_ALL;
>>@@ -3956,13 +3950,6 @@ void bond_setup(struct net_device *bond_dev)
>>	bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT;
>>	bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>>
>>-	/* At first, we block adding VLANs. That's the only way to
>>-	 * prevent problems that occur when adding VLANs over an
>>-	 * empty bond. The block will be removed once non-challenged
>>-	 * slaves are enslaved.
>>-	 */
>>-	bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
>>-
>>	/* don't acquire bond device's netif_tx_lock when
>>	 * transmitting */
>>	bond_dev->features |= NETIF_F_LLTX;
>>-- 
>>1.9.0
>>

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 27 Jun 2014 16:13:12 +0200

> This limitation maybe had some reason in the past, but now there is not
> one -> removing this.
> 
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>

Applied, thanks Jiri.

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Jiri Pirko]

Fri, Jun 27, 2014 at 06:08:50PM CEST, jay.vosburgh@canonical.com wrote:
>Tom Gundersen <teg@jklm.no> wrote:
>
>>Hi Jiri,
>>
>>On Fri, Jun 27, 2014 at 4:13 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> This limitation maybe had some reason in the past, but now there is not
>>> one -> removing this.
>>
>>If this restriction is really no longer needed, it would be very
>>helpful if we could drop it. See for instnace
>><https://bugs.freedesktop.org/show_bug.cgi?id=76077#c5>.
>
>	The reason for the restriction is that, in the past, the bond
>did not have a MAC address until the first slave was added, so adding a
>VLAN wouldn't work properly.
>
>	Currently, a random MAC is assigned to the bond when there are
>no slaves, so that problem is no longer an issue (as presumably the VLAN
>can handle the MAC change when the first slave goes in to the bond).
>
>	While you're at it, you might want to also check and possibly
>remove some of the MAC-related warnings related to removing the last
>slave, e.g.,
>
>        if (!bond_has_slaves(bond)) {
>                bond_set_carrier(bond);
>                eth_hw_addr_random(bond_dev);
>
>                if (vlan_uses_dev(bond_dev)) {
>                        pr_warn("%s: Warning: clearing HW address of %s while it still has VLANs\n",
>                                bond_dev->name, bond_dev->name);
>                        pr_warn("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs\n",
>                                bond_dev->name);
>                }
>        }
>
>	This warning may not be useful any longer, since the MAC should
>update correctly without user action when re-adding the first slave.

I just checked. The vlan dev holds its addr. So when new slave is added,
bond addr is changed to it, but vlan addr remains the same. So the
second warning still stands.

The first one can be removed/changed as hw addr is not cleared but
randomly generated.

>
>	-J
>
>
>>Cheers,
>>
>>Tom
>>
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>>  drivers/net/bonding/bond_main.c | 13 -------------
>>>  1 file changed, 13 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 3a451b6..ffefb70 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1001,12 +1001,6 @@ static netdev_features_t bond_fix_features(struct net_device *dev,
>>>         netdev_features_t mask;
>>>         struct slave *slave;
>>>
>>> -       if (!bond_has_slaves(bond)) {
>>> -               /* Disable adding VLANs to empty bond. But why? --mq */
>>> -               features |= NETIF_F_VLAN_CHALLENGED;
>>> -               return features;
>>> -       }
>>> -
>>>         mask = features;
>>>         features &= ~NETIF_F_ONE_FOR_ALL;
>>>         features |= NETIF_F_ALL_FOR_ALL;
>>> @@ -3956,13 +3950,6 @@ void bond_setup(struct net_device *bond_dev)
>>>         bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT;
>>>         bond_dev->priv_flags &= ~(IFF_XMIT_DST_RELEASE | IFF_TX_SKB_SHARING);
>>>
>>> -       /* At first, we block adding VLANs. That's the only way to
>>> -        * prevent problems that occur when adding VLANs over an
>>> -        * empty bond. The block will be removed once non-challenged
>>> -        * slaves are enslaved.
>>> -        */
>>> -       bond_dev->features |= NETIF_F_VLAN_CHALLENGED;
>>> -
>>>         /* don't acquire bond device's netif_tx_lock when
>>>          * transmitting */
>>>         bond_dev->features |= NETIF_F_LLTX;
>>> --
>>> 1.9.0
>
>---
>	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Michal Kubecek]

On Wed, Jul 02, 2014 at 10:46:52AM +0200, Jiri Pirko wrote:
> Fri, Jun 27, 2014 at 06:08:50PM CEST, jay.vosburgh@canonical.com wrote:
> >
> >        if (!bond_has_slaves(bond)) {
> >                bond_set_carrier(bond);
> >                eth_hw_addr_random(bond_dev);
> >
> >                if (vlan_uses_dev(bond_dev)) {
> >                        pr_warn("%s: Warning: clearing HW address of %s while it still has VLANs\n",
> >                                bond_dev->name, bond_dev->name);
> >                        pr_warn("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs\n",
> >                                bond_dev->name);
> >                }
> >        }
> >
> >	This warning may not be useful any longer, since the MAC should
> >update correctly without user action when re-adding the first slave.
> 
> I just checked. The vlan dev holds its addr. So when new slave is added,
> bond addr is changed to it, but vlan addr remains the same. So the
> second warning still stands.

Is it a problem? Since we have proper uc_list propagation, vlan should
work even if its address doesn't match the bond.

                                                        Michal Kubecek

Re: [patch net-next] bonding: allow to add vlans on top of empty bond

[Jiri Pirko]

Wed, Jul 02, 2014 at 11:05:17AM CEST, mkubecek@suse.cz wrote:
>On Wed, Jul 02, 2014 at 10:46:52AM +0200, Jiri Pirko wrote:
>> Fri, Jun 27, 2014 at 06:08:50PM CEST, jay.vosburgh@canonical.com wrote:
>> >
>> >        if (!bond_has_slaves(bond)) {
>> >                bond_set_carrier(bond);
>> >                eth_hw_addr_random(bond_dev);
>> >
>> >                if (vlan_uses_dev(bond_dev)) {
>> >                        pr_warn("%s: Warning: clearing HW address of %s while it still has VLANs\n",
>> >                                bond_dev->name, bond_dev->name);
>> >                        pr_warn("%s: When re-adding slaves, make sure the bond's HW address matches its VLANs\n",
>> >                                bond_dev->name);
>> >                }
>> >        }
>> >
>> >	This warning may not be useful any longer, since the MAC should
>> >update correctly without user action when re-adding the first slave.
>> 
>> I just checked. The vlan dev holds its addr. So when new slave is added,
>> bond addr is changed to it, but vlan addr remains the same. So the
>> second warning still stands.
>
>Is it a problem? Since we have proper uc_list propagation, vlan should
>work even if its address doesn't match the bond.

You are right, vlan_sync_address() should take care of it.

I'll send a patch removing these 2 warnings. Thanks.

>
>                                                        Michal Kubecek
>