[ovs-dev] [PATCH v2 8/8] datapath: Allow masks for set actions.

[Jesse Gross]

On Fri, Jul 18, 2014 at 8:15 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> On May 8, 2014, at 1:27 PM, Jesse Gross <jesse at nicira.com> wrote:
>
>> On Fri, Apr 11, 2014 at 4:29 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 logic for the tunnel restriction is that it is always cleared at
>> the start of the pipeline and therefore doesn't make sense to mask it?
>>
>
> Right, tunnel metadata is not transferred from input to output, so there is less need for masking for the tunnel info. Also in the case of multiple output actions to multiple tunnels the likelihood that most fields need to be set anyway is high.
>
>> I think it is good to avoid exception cases where possible even if it
>> doesn't necessarily make sense for the new action. For example, if
>> userspace generates masked actions then it could be convenient to do
>> that for all actions, even if the mask is always all bits set for
>> tunnels.
>>
>
> The implementation on the kernel module side seemed somewhat more complex, as the netlink data is nested, rather than a simple struct.

We convert the nested tunnel attributes to a simple struct at flow
setup time and masking would be at run time, so I wonder if we would
even need to care about the original format?

One difficulty that comes to mind is that for tunnel operations we
just set a pointer into the flow, which can be difficult for masks
since there isn't really anywhere to write the masked result to.
However, since we are already converting the tunnel mask actions at
flow setup and the initial state is known, one thought that comes to
mind is simply converting masked actions to non-masked during the
conversion.

I'm just trying to see if we can avoid transferring this complexity to
userspace.

>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>> index 0b66e7c..7cbd73c 100644
>>> --- a/datapath/actions.c
>>> +++ b/datapath/actions.c
>>> @@ -197,21 +228,28 @@ static void set_ipv6_addr(struct sk_buff *skb, u8 l4_proto,
>>>                          __be32 addr[4], const __be32 new_addr[4],
>>>                          bool recalculate_csum)
>>> {
>>> -       if (recalculate_csum)
>>> +       if (likely(recalculate_csum))
>>>                update_ipv6_checksum(skb, l4_proto, addr, new_addr);
>>
>> I would be somewhat more judicious with the use of likely() - it
>> doesn't do that much so I'm not sure that it really makes sense to use
>> it in places where we are essentially guessing what the user will do
>> or not do. It also makes it more difficult to review the patch because
>> there are additional extraneous changes (similarly with some of the
>> renaming).
>
> This change was based on my analysis of the code calling this static function. The cases where ‘recalculate_csum’ is simply ‘true’ are easy enough, but even in other cases it is false only if the ipv6 nexthdr is an extension header, and a routing header is found. I judged that for the majority of ipv6 packets this would not be the case. However, it seems that if this change is worthwhile, maybe I should put it to a separate patch?

OK, that sounds like a reasonable justification. But I agree that it
would be a little nicer in a separate patch.

>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index ea3cb79..ef6b569 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -585,7 +585,13 @@ struct ovs_action_recirc {
>>>  * indicate the new packet contents. This could potentially still be
>>>  * %ETH_P_MPLS if the resulting MPLS label stack is not empty.  If there
>>>  * is no MPLS label stack, as determined by ethertype, no action is taken.
>>> - * @OVS_ACTION_RECIRC: Recirculate within the data path.
>>> + * @OVS_ACTION_ATTR_RECIRC: Recirculate within the data path.
>>> + * @OVS_ACTION_ATTR_SET_MASKED: Replaces the contents of an existing header.  A
>>> + * nested %OVS_KEY_ATTR_* attribute specifies a header to modify, its value,
>>> + * and a mask.  For every bit set to one in the mask, the corresponding header
>>> + * field bit is set to the one in value, rest of the bits are left unchanged.
>>
>> I think the last sentence is a little confusing - when I read "to the
>> one in value", it sounded like the value must be one at first.
>>
>>> + * These non-significant bits must be passed in as zeroes, though.
>>
>> The zeroed non-significant bits are in the value, right?
>
> How about this:
>
>  * @OVS_ACTION_ATTR_SET_MASKED: Replaces the contents of an existing header.  A
>  * nested %OVS_KEY_ATTR_* attribute specifies a header to modify, its value,
>  * and a mask.  For every bit set in the mask, the corresponding bit value
>  * is copied from the value to the packet header field, rest of the bits are
>  * left unchanged.  The non-masked value bits must be passed in as zeroes.
>  * Masking is not supported for the %OVS_KEY_ATTR_TUNNEL attribute.

That is clearer to me, thanks.

Everything else sounds good to me.