From: Veaceslav Falico <vfalico@redhat•com>
To: Nikolay Aleksandrov <nikolay@redhat•com>
Cc: netdev@vger•kernel.org, Jay Vosburgh <fubar@us•ibm.com>,
Andy Gospodarek <andy@greyhouse•net>
Subject: Re: [PATCH v2 net-next 3/6] bonding: make bond_alb use 8021q's dev->vlan_info instead of vlan_list
Date: Fri, 9 Aug 2013 13:24:14 +0200 [thread overview]
Message-ID: <20130809112414.GC17510@redhat.com> (raw)
In-Reply-To: <5204CEF6.1030200@redhat.com>
On Fri, Aug 09, 2013 at 01:13:58PM +0200, Nikolay Aleksandrov wrote:
>On 08/08/2013 06:57 PM, Veaceslav Falico wrote:
>> RFC -> v1: use the changed __vlan_find_dev_next, which now works with
>> vlan's net_device instead of vlan's id. Also, fix a subtle race
>> condition if we remove the only vlan while looping through
>> MAX_LP_BURST - we end up with using the old vlan_id, so set it
>> to 0 if we don't have vlans.
>> v1 -> v2: no change.
>>
>> In alb mode, we only need each vlan's id (that is on top of bond) to tag
>> learning packets, so get them via __vlan_find_dev_next(bond->dev, last_dev).
>>
>> We must also find *any* vlan (including last id stored in current_alb_vlan)
>> if we can't find anything >= current_alb_vlan id.
>>
>> For that, we verify if bond has any vlans at all, and if yes - find the
>> next vlan id after current_alb_vlan id. So, if vlan id is not 0, we tag the
>> skb.
>>
>> CC: Jay Vosburgh <fubar@us•ibm.com>
>> CC: Andy Gospodarek <andy@greyhouse•net>
>> Signed-off-by: Veaceslav Falico <vfalico@redhat•com>
>> ---
>> drivers/net/bonding/bond_alb.c | 47 ++++++++++++++++++++++++++++-----------
>> drivers/net/bonding/bond_alb.h | 2 +-
>> 2 files changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index 2684329..ced5753 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -974,8 +974,9 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
>> {
>> struct bonding *bond = bond_get_bond_by_slave(slave);
>> struct learning_pkt pkt;
>> + struct net_device *vlan_dev;
>> int size = sizeof(struct learning_pkt);
>> - int i;
>> + int i, vlan_id;
>Styling nitpick: maybe re-arrange them longest -> shortest ?
Yep, sure, thank you.
>
>>
>> memset(&pkt, 0, size);
>> memcpy(pkt.mac_dst, mac_addr, ETH_ALEN);
>> @@ -1000,22 +1001,42 @@ static void alb_send_learning_packets(struct slave *slave, u8 mac_addr[])
>> skb->priority = TC_PRIO_CONTROL;
>> skb->dev = slave->dev;
>>
>> + rcu_read_lock();
>Would've saved a couple of lines if we could move the rcu_read_lock inside the
>if {} block but I know that you're keeping the bond_vlan_used() result here, but
>I think that with the new functions we're covered i.e. if the vlan has
>disappeared between the call to bond_vlan_used and the afterwards we'll get NULL
>and just drop the packet, but this can be reworked to continue with the packet
>without a vlan if one is not found that is to send it without tag :-)
Yeah, not a lot of sense to hold it, however I don't see right now how it
might save us lines - we need to hold vlan_dev if it's found, and need to
release the lock if it's NULL (before continue).
>> if (bond_vlan_used(bond)) {
>> - struct vlan_entry *vlan;
>> -
>> - vlan = bond_next_vlan(bond,
>> - bond->alb_info.current_alb_vlan);
>> -
>> - bond->alb_info.current_alb_vlan = vlan;
>> - if (!vlan) {
>> + /* first try to find the previously used vlan by
>> + * id, which might have gone away already
>> + */
>> + vlan_id = bond->alb_info.current_alb_vlan;
>> + vlan_dev = __vlan_find_dev_deep(bond->dev,
>> + htons(ETH_P_8021Q),
>> + vlan_id);
>> +
>> + /* search for the next one, if not found - for any */
>> + if (vlan_dev)
>> + vlan_dev = __vlan_find_dev_next(bond->dev,
>> + vlan_dev);
>> + if (!vlan_dev)
>> + vlan_dev = __vlan_find_dev_next(bond->dev,
>> + NULL);
>> +
>This part really looks ambiguous, I've left a comment that concerns it in my
>reply to patch 02.
Yep, seen that, it indeed is not quite clear (though working when you think
a bit about it :) ). I'll address your comment about 02/06 in several days,
I'm travelling now.
>
>> + if (vlan_dev) {
>> + vlan_id = vlan_dev_vlan_id(vlan_dev);
>> + bond->alb_info.current_alb_vlan = vlan_id;
>> + } else {
>> + bond->alb_info.current_alb_vlan = 0;
>> + rcu_read_unlock();
>> kfree_skb(skb);
>> continue;
>> }
>> + } else
>> + vlan_id = 0;
>Styling nitpick: if {} else {}
Yep, thanks.
>
>Cheers,
> Nik
next prev parent reply other threads:[~2013-08-09 11:25 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-08 16:57 [PATCH net-next v2 0/6] bonding: remove bond->vlan_list Veaceslav Falico
2013-08-08 16:57 ` [PATCH v2 net-next 1/6] bonding: add rcu to vlan_uses_dev() and make bond_vlan_used() use it Veaceslav Falico
2013-08-09 11:06 ` Nikolay Aleksandrov
2013-08-09 11:11 ` Veaceslav Falico
2013-08-08 16:57 ` [PATCH v2 net-next 2/6] vlan: add __vlan_find_dev_next() Veaceslav Falico
2013-08-09 7:30 ` Veaceslav Falico
2013-08-09 11:07 ` Nikolay Aleksandrov
2013-08-14 15:28 ` Veaceslav Falico
2013-08-08 16:57 ` [PATCH v2 net-next 3/6] bonding: make bond_alb use 8021q's dev->vlan_info instead of vlan_list Veaceslav Falico
2013-08-09 11:13 ` Nikolay Aleksandrov
2013-08-09 11:24 ` Veaceslav Falico [this message]
2013-08-08 16:57 ` [PATCH v2 net-next 4/6] bonding: convert bond_has_this_ip to use bond->dev->vlan_info Veaceslav Falico
2013-08-08 16:57 ` [PATCH v2 net-next 5/6] bonding: convert bond_arp_send_all " Veaceslav Falico
2013-08-09 11:42 ` Nikolay Aleksandrov
2013-08-08 16:57 ` [PATCH v2 net-next 6/6] bonding: remove unused bond->vlan_list Veaceslav Falico
2013-08-09 11:44 ` Nikolay Aleksandrov
2013-08-26 16:31 ` [PATCH net-next v2 0/6] bonding: remove bond->vlan_list Veaceslav Falico
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=20130809112414.GC17510@redhat.com \
--to=vfalico@redhat$(echo .)com \
--cc=andy@greyhouse$(echo .)net \
--cc=fubar@us$(echo .)ibm.com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@redhat$(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