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.k...@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@openvswitch.org
> >>>http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
