[ovs-dev] [PATCH 07/10] lib/odp: Use masked set actions.

[Ben Pfaff]

On Tue, Apr 08, 2014 at 04:38:49PM -0700, Jarno Rajahalme wrote:
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

Consider this code from commit_set_ether_addr_action():

    bool have_mask_src = !eth_addr_is_zero(wc->masks.dl_src);
    bool have_mask_dst = !eth_addr_is_zero(wc->masks.dl_dst);

...

    memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
    memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);

    /* Do not use mask if fully masked. */
    if (use_masked && !(have_mask_src && have_mask_dst)) {
        memcpy(mask.eth_src, wc->masks.dl_src, ETH_ADDR_LEN);
        eth_addr_bitand(base->dl_src, wc->masks.dl_src, key.eth_src);
        memcpy(mask.eth_dst, wc->masks.dl_dst, ETH_ADDR_LEN);
        eth_addr_bitand(base->dl_dst, wc->masks.dl_dst, key.eth_dst);

        commit_masked_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET, &key,
                                 &mask, sizeof key);
    } else {
        memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
        memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);

        memcpy(key.eth_src, base->dl_src, ETH_ADDR_LEN);
        memcpy(key.eth_dst, base->dl_dst, ETH_ADDR_LEN);

        commit_set_action(odp_actions, OVS_KEY_ATTR_ETHERNET, &key,
                          sizeof key);
    }

First, !(have_mask_src && have_mask_dst) is stricter than it would
have to be, right?  It sets all the bits in the Ethernet addresses if
at least one bit in dl_src needs to be set and at least one bit in
dl_dst needs to be set.  It could, instead, set all the bits if *all*
the bits in dl_src and dl_dst need to be set.  Now, I'm not sure that
it's worth doing that, since it's an extra check, but I do want to
make sure that I understand the logic.

Second, 'base' gets used a lot in the "if" statement.  I guess that's
OK, because the previous memcpys mean that it has the same contents as
'flow' for dl_src and dl_dst, but 'base' is used only in special
situations (where the previous value of a field matters) so it stuck
out, and it took me some time to realize that it was in fact the same
as 'flow'.  So, I'd prefer to see 'flow' used, where it can be, in
place of 'base'.  I feel the same way about some other cases later,
e.g. commit_set_ipv4_action().

Finally, commit_masked_set_action() is used here and elsewhere in kind
of an awkward pattern, where the caller has to do masking of the key
against the mask.  I think that commit_masked_set_action() could do
the masking as it copies the key, and then all of the callers would be
simpler.  (If you don't take my advice on that, then I think that
commit_set_priority_action() and commit_set_pkt_mark_action() forgot
to do the masking.)

There is a doubled ';' in commit_set_ipv4_action():
+            key.ipv4_src = base->nw_src & wc->masks.nw_src;;

The beginning of commit_set_ipv4_action() first asserts:
        !((a ^ b) & ~c)
and then checks whether
        (a ^ b) & c
for several instances of 'a', 'b', and 'c'.  But if the assertions are
true, then I believe that the checks can be reduced to just
        a != b
because we already know at that the bits not in 'c' are the same.  (Am
I missing something?)

It seems to me that IPV6_MASKED could be written as a function.  At
least, the uses of the macro arguments should be (parenthesized).

In commit_set_ipv6_action(), is the following mask check needed?  I
doubt that it is much faster than the check that follows it, and if
that later checks passes (does not "return;") then we know that some
bits in the mask must be set.  Same question for the early check in
commit_set_arp_action(), commit_set_port_action(), ...

    /* Wildcard bits are set when we have either read or set the corresponding
     * values.  Nothing to be done if no bits are set. */
    if (!(ipv6_mask_is_any(&wc->masks.ipv6_src) ||
          ipv6_mask_is_any(&wc->masks.ipv6_dst) ||
          wc->masks.ipv6_label || wc->masks.nw_tos || wc->masks.nw_ttl)) {
        return;
    }

commit_set_arp_action() may always use masked "set" operations,
regardless of 'use_masked', because ARP set operations are always
slow-pathed (see the "return SLOW_ACTION;" at the end of the
function).

Thanks,

Ben.