Ben,
Thanks for your review. Some comments below,
Jarno
On Apr 9, 2014, at 1:19 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Tue, Apr 08, 2014 at 04:38:49PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme <jrajaha...@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.
You are right, my intention was to “not use mask if fully masked”. The thought
here is that if ethernet addresses are set, it is most likely that both are
fully set at the same time, and it would be slightly faster in the datapath to
not have to bother about the mask. We could, however, let the datapath to worry
about this, as it is validating the actions. Indeed, it could be faster for the
datapath to implement field-spcific set actions internally for the typical
cases.
>
> 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().
Will change.
>
> 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.)
>
True, it will be simpler and cleaner.
> 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?)
>
I had intended to remove the assertions. However, those have been valuable in
catching cases where flow values have been modified without setting the mask
bits. One way of keeping them around for testing purposes would be to piggyback
on the FLOW_WC_SEQ, in effect enabling the asserts every time FLOW_WC_SEQ is
bumped… other ideas?
> 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;
> }
>
I’ll look into removing these.
> 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).
>
OK.
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
