I cleaned up the obvious items you mentioned.
> In add_mirror_actions(), I think that the top-level assignment to
> ctx->mirrors should be removed, in favor of adding
> ctx->mirrors |= m->dup_mirrors;
> after the VLAN test inside the loop. That way, mirrors that are not
> actually used because the VLAN criteria are not met are not accounted
> packets that they don't receive.
Make sense. Thanks.
> The output logic near the end of xlate_normal() looks wrong to me
> now. Previously, if the MAC table said that an incoming packet should
> go out the port it came in on, we dropped. I believe that the new
> logic will instead flood it. I think that the fix is for:
> if (mac && mac->port.p != in_bundle) {
> output_normal(ctx, mac->port.p, vlan);
> to become:
> if (mac) {
> if (mac->port.p != in_bundle) {
> output_normal(ctx, mac->port.p, vlan);
> }
Great catch. Thank you.
> I don't think that assumption is actually true, though. I see random
> order when I add more than one port at a time. I think that you are
> actually just getting really "lucky". The order of the ports in the
> hash table depends on the low bits of the hash value of the name.
> When there are 4 ports (br0, p1, p2, p3) there will be 4 buckets in
> the hash table and so the low 2 bits are important. You happen to be
> really lucky with the hash function:
>
> (gdb) p hash_bytes("p1", 2, 0) & 3
> $2 = 0
> (gdb) p hash_bytes("p2", 2, 0) & 3
> $3 = 1
> (gdb) p hash_bytes("p3", 2, 0) & 3
> $4 = 3
>
> so that hash(p1) < hash(p2) < hash(p3) when we only look at the low 2
> bits.
>
> The hash function isn't stable across endianness so this probably
> isn't true on SPARC. Even on x86, if I change "p1" to "x" then I
> start seeing test failures.
Wow, that was some impressive forensic debugging. Thanks for pointing that
out. I reworked the tests so they should be portable. I plan to add a bunch
more tests next week.
Thanks for the review. I pushed the series.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
