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

[Jarno Rajahalme]

Jesse,

Thanks for the review, and sorry for the time it has taken me to get back to this! Responses below,

  Jarno

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.

> In any case, if we do need to make such a restriction then it should
> be flagged at flow installation time, rather than runtime as appears
> to be the case now.
> 

I’ll make it so.

>> It is not clear whether 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.
> 
> I think it's good to support it for the reasons above.
> 
> The other main thing that I noticed is that this results in a fair
> amount of code duplication in actions - particularly with things like
> checking for appropriate length and updating checksums (and having two
> copies of the IPv6 code is really painful). It seems like the ideal
> thing to do is to translate non-masked actions into masked at
> validation time, although I know that could potentially be a pain.

I thought so too, but it ended up being rather simple after all. I’ll post a new version where tunnel action is executed without a mask, and all others with a mask by converting to masked set action at the validation time.

> Otherwise, I also like what you did with SCTP.

> 
>> 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?

> 
>> @@ -235,22 +301,26 @@ static int set_ipv4(struct sk_buff *skb, const struct ovs_key_ipv4 *ipv4_key)
>> 
>>        nh = ip_hdr(skb);
>> 
>> -       if (ipv4_key->ipv4_src != nh->saddr)
>> -               set_ip_addr(skb, nh, &nh->saddr, ipv4_key->ipv4_src);
>> +       if (unlikely(mask->ipv4_src))
>> +               set_ip_addr(skb, nh, &nh->saddr,
>> +                           key->ipv4_src | (nh->saddr & ~mask->ipv4_src));
>> 
>> -       if (ipv4_key->ipv4_dst != nh->daddr)
>> -               set_ip_addr(skb, nh, &nh->daddr, ipv4_key->ipv4_dst);
>> +       if (unlikely(mask->ipv4_dst))
>> +               set_ip_addr(skb, nh, &nh->daddr,
>> +                           key->ipv4_dst | (nh->daddr & ~mask->ipv4_dst));
> 
> It might be worthwhile to have a u32 mask function/macro as it seems
> to come up a lot (maybe u16 as well for the ports).

It seems a generic macro will do as the operation pattern is the same regardless of the type.

> 
>> -       if (ipv4_key->ipv4_tos != nh->tos)
>> -               ipv4_change_dsfield(nh, 0, ipv4_key->ipv4_tos);
>> +       if (mask->ipv4_tos)
>> +               ipv4_change_dsfield(nh, 0,
>> +                                   key->ipv4_tos | (nh->tos & ~mask->ipv4_tos));
> 
> The second argument of ipv4_change_dsfield() is actually a mask, so we
> can simplify this slightly.

Thanks for pointing this out.

> 
>> 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.

  Jarno