[ovs-dev] Unwildcarding of fields modified by set_field action

[Jan Scheurich]

Thanks Jarno, I understand the issue and the need for tracking the set 
mask now.

We will stick to the post-xlate cleanup for the time being and possibly 
return to introducing a dedicated set-mask at some later stage.

Jan

On 2017-01-12 01:04, Jarno Rajahalme wrote:
>> On Jan 11, 2017, at 12:10 AM, Jan Scheurich <jan.scheurich at web.de> wrote:
>>
>>
>> On 2017-01-11 01:14, Jarno Rajahalme wrote:
>>> Unwildcarding of set fields is only done for practical reasons in the userspace code, and could be optimized away, especially for recent enough datapaths that implement the masked set action.
>>>
>>> Currently, the same bit mask is used to mark what fields have been masked and what fields have been set. It would be possible to add a separate flow mask for setting and use the current one only for matching. It will cause some additional overhead on the upcall processing, but will generate better megaflows. I remember looking at this when implementing datapath masked set actions, and vaguely recall someone else also having a go at this later?
>> Not quite sure I understand why set fields have to be tracked. Can you explain a bit and point me to the place in the code where the data in wc is used in this respect?
> OVS OpenFlow action processing does not directly execute set actions on a packet, but on the flow key. Later, in commit_odp_actions(), we translate any differences between base_flow and flow key into datapath set actions.  In general, datapath set actions act on multiple fields at the same time (e.g., Ethernet addresses, IP fields, or L4 ports). Staging the changes made by OpenFlow actions to the flow key allows coalescing multiple such changes to less granular datapath actions.
>
> You could look at commit_set_ipv4_action() in lib/odp-util.c for a specific example.
>
>>>> In the context of L3 tunneling and PTAP the current behavior produces incorrect results because setting the dl_dst of a packet received from an L3 port adds a match on dl_dst==00:00:00:00:00:00 to the Megaflow which can never match because the packet received from the L3 tunnel does not have a dl_dst field.
>>>>
>>>> We might have some code sanitizing the mask that may take care of this, as there are also other cases where we are setting fields that do not exist in the incoming packet.
>> Yes, that's what we have done now in xlate_wc_finish() to clean up. But my gut feeling is that this shouldn't be needed in the first place. Maybe I'm still missing some essential piece?
>>
> Consider any action that pushes headers to the current packet and then sets any of the new fields, and/or later stages of the pipeline (maybe through a patch port) match on those fields. Those fields do not exist in the incoming packet, and thus need to be excluded from the datapath flow mask. Generally, mask fields should be specified only for fields that exist in the flow key included in the upcall.
>
>    Jarno
>
>> Regards, Jan
>>