[ovs-dev] [PATCH 1/1] bond: Fixing bond hash tag usage

Zoltan Kiss zoltan.kiss at citrix.com
Thu Jan 3 17:15:35 UTC 2013


Hi,

I've tested with your fix (865f22b3b3cb9), and it solves the problem 
indeed. My approach to fix the problem is based on the comment of the 
tag member:

/* A hash bucket for mapping a flow to a slave.
  * "struct bond" has an array of (BOND_MASK + 1) of these. */
struct bond_entry {
     struct bond_slave *slave;   /* Assigned slave, NULL if unassigned. */
...
     tag_type tag;               /* Tag for entry<->slave association. */

My understanding was that the hash is associated to the slave by this 
field, however it doesn't make too much sense as 'slave' already do 
this. Your commit message on 865f22b3b3cb9 cleared the picture, I 
suggest to clarify this comment for bond_entry:tag.
My solution also works but it forces ofproto to revalidate every other 
hash on the same slave, which is not necessary as their slave did not 
change. You can drop it.

Regards,

Zoli

On 31/12/12 17:14, Ben Pfaff wrote:
> Oh, also you said you tested it against OVS 1.4.2, but not against
> 1.4.3, which has an important relevant fix as commit 865f22b3b3cb9
> (bond: Tag flows according to their hash bucket, not just their slave.).
> It's always best to test against the latest version in whatever branch
> you're using.
>
> On Mon, Dec 31, 2012 at 09:12:11AM -0800, Ben Pfaff wrote:
>> The patch doesn't entirely add up.  I've been intending to test the
>> behavior myself, but with the holidays and other stuff I have to do, I
>> haven't managed it yet.
>>
>> On Mon, Dec 31, 2012 at 03:48:12PM +0000, Zoltan Kiss wrote:
>>> Hi,
>>>
>>> Did you guys had any chance to check this patch? (and the one
>>> related in the next mail)
>>> Btw. the numbering in the subject is wrong, it should be [PATCH 1/2].
>>>
>>> Zoli
>>>
>>> On 20/12/12 21:39, Zoltan Kiss wrote:
>>>> As the description of this field states, it should link the hash entry
>>>> to the slave. With random tags, the flow revalidation never finds the
>>>> corresponding facet after bond rebalancing, therefore the flow
>>>> entries never get updated.
>>>> I've tested it on Xenserver 6.1 (OVS 1.4.2), two NIC with LACP bonding
>>>> and 4 VMs running iperf client against iperf servers running on two
>>>> separate hosts on the same Ethernet switch (1 Gbps connection each).
>>>> Running a similar command like the following during test
>>>> running helps you to see whether the flow entries followed the
>>>> rebalancing or not:
>>>>
>>>> ovs-dpctl dump-flows xapi1|grep dst=5001; ovs-appctl bond/show bond0
>>>>
>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss at citrix.com>
>>>> ---
>>>>   lib/bond.c | 4 ++--
>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/bond.c b/lib/bond.c
>>>> index 2c59f9d..ba98fff 100644
>>>> --- a/lib/bond.c
>>>> +++ b/lib/bond.c
>>>> @@ -739,7 +739,7 @@ bond_shift_load(struct bond_entry *hash, struct bond_slave *to,
>>>>       /* Arrange for flows to be revalidated. */
>>>>       tag_set_add(set, hash->tag);
>>>>       hash->slave = to;
>>>> -    hash->tag = tag_create_random();
>>>> +    hash->tag = to->tag;
>>>>   }
>>>>
>>>>   /* Pick and returns a bond_entry to migrate to 'to' (the least-loaded slave),
>>>> @@ -1446,7 +1446,7 @@ choose_output_slave(const struct bond *bond, const struct flow *flow,
>>>>               if (!e->slave->enabled) {
>>>>                   e->slave = bond->active_slave;
>>>>               }
>>>> -            e->tag = tag_create_random();
>>>> +            e->tag = e->slave->tag;
>>>>           }
>>>>           *tags |= e->tag;
>>>>           return e->slave;
>>>>
>>>
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list