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

Ben Pfaff blp at nicira.com
Thu Jan 3 18:22:45 UTC 2013


Thanks for confirming the fix.  I'll take a look at the second patch
then.

On Thu, Jan 03, 2013 at 05:15:35PM +0000, Zoltan Kiss wrote:
> 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