[ovs-dev] [PATCH 09/10] datapath: Allow masks for set actions.
Thomas Graf
tgraf at redhat.com
Wed Apr 9 09:53:40 UTC 2014
On 04/09/2014 01:38 AM, Jarno Rajahalme wrote:
> Masked set actions allow more megaflow wildcarding. All other key
> types than the tunnel key that can be set, can now be set with a mask.
>
> It is not clear wether masked set is useful for skb_priority.
> However, we already use the LSB of pkt_mark for IPSec in tunnels, so
> it might be useful to be able to set individual bits on pkt_mark.
This looks great!
> + if (mask) {
> + ether_addr_copy_masked(eth_hdr(skb)->h_source, key->eth_src,
> + mask->eth_src);
> + ether_addr_copy_masked(eth_hdr(skb)->h_dest, key->eth_dst,
> + mask->eth_dst);
> + } else {
> + ether_addr_copy(eth_hdr(skb)->h_source, key->eth_src);
> + ether_addr_copy(eth_hdr(skb)->h_dest, key->eth_dst);
> + }
Quite a few of these branches are introduced in the fast path. Might
be worth checking 'perf stat' quickly to see if this generates a lot
of branch misses for either the wildcards enabled or the wildcards
disabled case. It might in fact be faster to always assume a mask if
the compiler cannot predict this correctly.
> @@ -1281,13 +1294,17 @@ static int validate_set(const struct nlattr *a,
> {
> const struct nlattr *ovs_key = nla_data(a);
> int key_type = nla_type(ovs_key);
> + bool have_mask;
>
> /* There can be only one key in a action */
> if (nla_total_size(nla_len(ovs_key)) != nla_len(a))
> return -EINVAL;
>
> + have_mask = (ovs_key_lens[key_type] * 2 == nla_len(ovs_key));
> +
key_type is not validated at this point and might be > OVS_KEY_ATTR_MAX
and thus access ovs_key_lens[] out of bound.
> if (key_type > OVS_KEY_ATTR_MAX ||
> (ovs_key_lens[key_type] != nla_len(ovs_key) &&
> + (!have_mask || !validate_masked(nla_data(ovs_key), nla_len(ovs_key))) &&
> ovs_key_lens[key_type] != -1))
> return -EINVAL;
>
More information about the dev
mailing list