From: Jiri Pirko <jiri@resnulli•us>
To: Jay Vosburgh <jay.vosburgh@canonical•com>
Cc: Tom Gundersen <teg@jklm•no>, netdev <netdev@vger•kernel.org>,
David Miller <davem@davemloft•net>,
vfalico@gmail•com, andy@greyhouse•net
Subject: Re: [patch net-next] bonding: allow to add vlans on top of empty bond
Date: Wed, 2 Jul 2014 10:46:52 +0200 [thread overview]
Message-ID: <20140702084652.GA3015@minipsycho.orion> (raw)
In-Reply-To: <26863.1403885330@localhost.localdomain>
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
next prev parent reply other threads:[~2014-07-02 8:46 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-27 14:13 [patch net-next] bonding: allow to add vlans on top of empty bond Jiri Pirko
2014-06-27 15:27 ` Tom Gundersen
2014-06-27 16:08 ` Jay Vosburgh
2014-07-02 8:46 ` Jiri Pirko [this message]
2014-07-02 9:05 ` Michal Kubecek
2014-07-02 9:13 ` Jiri Pirko
2014-06-28 7:30 ` Veaceslav Falico
2014-06-28 7:49 ` Jiri Pirko
2014-07-02 1:58 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140702084652.GA3015@minipsycho.orion \
--to=jiri@resnulli$(echo .)us \
--cc=andy@greyhouse$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=jay.vosburgh@canonical$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=teg@jklm$(echo .)no \
--cc=vfalico@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox