[ovs-dev] [mirrors 3/3] mirroring: Don't require the "normal" action to perform mirring.

[Ben Pfaff]

On Wed, Nov 23, 2011 at 03:14:06PM -0800, Justin Pettit wrote:
> Previously, mirrors only worked when using the "normal" action.  This
> commit performs mirroring even when mirroring is not used.  It also adds
> some unit tests.

s/mirring/mirroring/ in subject.

"git am" says:

    Applying: mirroring: Don't require the "normal" action to perform mirring.
    /home/blp/db/.git/rebase-apply/patch:357: trailing whitespace.
      [Datapath actions: 1 
    /home/blp/db/.git/rebase-apply/patch:391: trailing whitespace.
      [Datapath actions: 1 
    warning: 2 lines add whitespace errors.

I think that the NEWS item should spell out the change better.  I
don't think that it does a good job of explaining for anyone not up on
OpenFlow and OVS jargon.

I am very pleased that OFBUNDLE_FLOOD is gone.

In add_mirror_actions(), in the loop over actions, it's possible for
'ofport' to be NULL, and in that case we should avoid dereferencing
it.

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.

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);
        }

In the comment on the mirror_set member function, I'd s/support
it/support mirroring/.

The tests have this comment:
    dnl This test assumes that OpenFlow port numbers are allocated in order
    dnl starting from one.  Unlike the previous tests, it does require that
    dnl they are allocated in the same order that they are named in the
    dnl database.

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.

The VLAN test checks the port numbers and uses shell variables to
represent them:

    AT_CHECK(
      [ovs-vsctl \
	    -- get Interface p1 ofport \
	    -- get Interface p2 ofport \
	    -- get Interface p3 ofport \
	    -- get Interface p4 ofport \
	    -- get Interface p5 ofport \
	    -- get Interface p6 ofport \
	    -- get Interface p7 ofport \
	    -- get Interface p8 ofport],
      [0], [stdout])
    set `cat stdout`
    br0=0 p1=$1 p2=$2 p3=$3 p4=$4 p5=$5 p6=$6 p7=$7 p8=$8

Thanks,

Ben.