I’ve just posted a v3 of this series,
Jarno
On Aug 5, 2014, at 10:57 AM, Jesse Gross <je...@nicira.com> wrote:
> On Fri, Jul 18, 2014 at 8:15 AM, Jarno Rajahalme <jrajaha...@nicira.com>
> wrote:
>> On May 8, 2014, at 1:27 PM, Jesse Gross <je...@nicira.com> wrote:
>>
>>> On Fri, Apr 11, 2014 at 4:29 PM, Jarno Rajahalme <jrajaha...@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?
The original format requires masked set action key data to be immediately
followed by the mask, within the same netlink attribute. For nested netlink
attributes this would probably mean that each inner netlink attribute must have
the mask following the data. As no tunnel info is retained from the input side,
support for masked tunnel info would have no benefit, as masked set tunnel
action would make no difference on the flow wildcards.
>
> 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.
>
The tunnel key is already a special case for the input/output issue referenced
above, also for userspace. Set tunnel action is triggered for tunnel output
only, i.e. even if tunnel ID is set, it has no effect unless the packet is
output to a tunnel port. think leaving the set tunnel action out of the masked
set action is not making this difference any worse.
>>>> 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.
v3 has this 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
