[ovs-dev] [PATCH v5 11/13] datapath: Allow masks for set actions.

[Jarno Rajahalme]

On Nov 25, 2014, at 7:33 PM, Jesse Gross <jesse at nicira.com> wrote:

> On Fri, Sep 5, 2014 at 4:05 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> Masked set action allows more megaflow wildcarding.  Masked set action
>> is now supported for all writeable key types, except for the tunnel
>> key.
>> 
>> The set tunnel action is an exception as any input tunnel info is
>> cleared before action processing starts, so there is no tunnel info to
>> mask.
>> 
>> The kernel module converts all (non-tunnel) set actions to masked set
>> actions.  This makes action processing more uniform, and results in
>> less branching and duplicating the action processing code.  When
>> returning actions to userspace, the fully masked set actions are
>> converted to normal set actions.  To make this mapping consistent, we
>> reject fully masked set actions at flow set-up time.
>> 
>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> Sorry about the extremely long delay, I'm finally coming back to this…

No worries :-)

> 
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index 43ca2a0..243b672 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> +static int set_ipv6(struct sk_buff *skb, const struct ovs_key_ipv6 *key,
>> +                   const struct ovs_key_ipv6 *mask)
>> {
>> @@ -483,38 +525,56 @@ static int set_ipv6(struct sk_buff *skb, const struct ovs_key_ipv6 *ipv6_key)
>> -       if (memcmp(ipv6_key->ipv6_dst, daddr, sizeof(ipv6_key->ipv6_dst))) {
>> +               mask_ipv6_addr(saddr, key->ipv6_src, mask->ipv6_src, masked);
>> +
>> +               if (unlikely(memcmp(saddr, masked, sizeof masked))) {
>> +                       set_ipv6_addr(skb, key->ipv6_proto, saddr, masked,
>> +                                     true);
> 
> set_ipv6_addr() does a memcpy as well, I think that's now unnecessary
> given that we are masking into the packet directly.

The above actually masks to a temporary first (‘masked’), then compares the ‘masked’ against packet’s address (‘saddr’), and only modifies the packet if needed, avoiding checksum update and skb_clear_hash in the common case (recall that the userspace currently will ‘set’ all fields that are being matched as a side effect of using flow wildcards mask for keeping track of both matches and sets).

The common case could be faster if I used a masked compare first, and mask the address and call set_ipv6_addr(). inet_proto_csum_replace16() currently needs full old and new addresses so I see no obvious way to masking in-place without additional copying.

> 
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 69d1919..ec32a00 100644
>> --- a/datapath/flow_netlink.c
>> +++ b/datapath/flow_netlink.c
>> @@ -1682,12 +1743,45 @@ static int validate_set(const struct nlattr *a,
>> +       /* Convert non-masked non-tunnel set actions to masked set actions. */
>> +       if (!masked && key_type != OVS_KEY_ATTR_TUNNEL) {
>> +               int start, len = key_len * 2;
>> +               struct nlattr *at;
>> +
>> +               *skip_copy = true;
>> +
>> +               start = add_nested_action_start(sfa, OVS_ACTION_ATTR_SET_MASKED);
>> +               if (start < 0)
>> +                       return start;
>> +
>> +               at = __add_action(sfa, key_type, NULL, len);
>> +               if (IS_ERR(at))
>> +                       return PTR_ERR(at);
>> +
>> +               memcpy(nla_data(at), nla_data(ovs_key), key_len); /* Key. */
>> +               /* Even though all-ones mask includes non-writeable fields,
>> +                * which we do not allow above, we will not actually set them
>> +                * when we execute the masked set action. */
>> +               memset(nla_data(at) + key_len, 0xff, key_len);    /* Mask. */
> 
> What about IPv6 flow label? I think we need to actually unmask those
> bits since we want to retain the original value in the packet instead
> of zeroing them.
> 

Right, my comment above was wrong regarding the bits 21-24. How about this incremental:

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 0fc4c61..0363dc2 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1802,11 +1802,13 @@ static int validate_set(const struct nlattr *a,
 			return PTR_ERR(at);
 
 		memcpy(nla_data(at), nla_data(ovs_key), key_len); /* Key. */
-		/* Even though all-ones mask includes non-writeable fields,
-		 * which we do not allow above, we will not actually set them
-		 * when we execute the masked set action. */
 		memset(nla_data(at) + key_len, 0xff, key_len);    /* Mask. */
+		/* Clear non-writeable bits from otherwise writeable fields. */
+		if (key_type == OVS_KEY_ATTR_IPV6) {
+			struct ovs_key_ipv6 *mask = nla_data(at) + key_len;
 
+			mask->ipv6_label &= htonl(0x000FFFFF);
+		}
 		add_nested_action_end(*sfa, start);
 	} else if (masked) {
 		/* Reject fully masked masked set actions so that the above


>> +       } else if (masked) {
>> +               /* Reject fully masked masked set actions so that the above
>> +                * conversion can be unabiguously reverted when sending
>> +                * actions back to userspace. */
>> +               if (is_all_ones(nla_data(ovs_key) + key_len, key_len))
>> +                       return -EINVAL;
> 
> It kind of pains me to have these types of exceptions from the
> perspective of the new protocol that we are introducing. Is there any
> way to achieve this effect without introducing a user-visible
> interface quirk?

We could have another, kernel internal action code (e.g., OVS_ACTION_ATTR_SET_TO_MASKED), that would execute just like OVS_ACTION_ATTR_SET_MASKED, but would retain the fact that it was translated, so we could translate it back when needed.

  Jarno