On 1/4/24 07:35, Eli Britstein via discuss wrote:
>
>
>> -----Original Message-----
>> From: Ilya Maximets <i.maxim...@ovn.org>
>> Sent: Wednesday, 3 January 2024 19:12
>> To: Eli Britstein <el...@nvidia.com>; ovs-discuss@openvswitch.org
>> Cc: Maor Dickman <ma...@nvidia.com>; i.maxim...@ovn.org
>> Subject: Re: [ovs-discuss] Match on header rewrite fields
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 12/19/23 17:00, Eli Britstein via discuss wrote:
>>> Hello,
>>>
>>> In case there is a header rewrite action, OVS implicitly adds match on the
>> corresponding fields.
>>>
>>> For example:
>>> ovs-ofctl add-flow br-int0
>> 'in_port=enp8s0f0,ip,actions=mod_dl_dst=00:11:22:33:44:55,enp8s0f0_0'
>>>
>>> The datapath flow will have an exact match on dl_dst field.
>>>
>>> Does someone know what is the logic behind this?
>>
>> Hi, Eli. The main reason for this, AFAIK, is that OVS is actually using the
>> field
>> value while trying to optimize the action later. If the action doesn't
>> really
>> change the value, the action will be omitted. In this case, if we do not
>> have a
>> match on the value, the action will not be executed for other packets as
>> well.
>> And that is a problem. It's also hard to debug such issues, because it
>> depend
>> on which packet hits OVS first.
>>
>> E.g. if the packet with dl_dst=00:11:22:33:44:55 hits OVS, OVS will install
>> the
>> following datapath flow:
>>
>> in_port(enp8s0f0),eth(dst=00:11:22:33:44:55), actions:enp8s0f0_0
>>
>> For a packet with dl_dst=00:11:22:33:44:77, it will be:
>>
>> in_port(enp8s0f0),eth(dst=00:11:22:33:44:77),
>> actions:set(eth(dst=00:11:22:33:44:55)),enp8s0f0_0
>>
>> If we remove the match, the first flow will turn into:
>>
>> in_port(enp8s0f0), actions:enp8s0f0_0
>>
>> swallowing all the traffic without modifying the fields, if it happens to be
>> installed first.
>>
>> There might be a room for optimization, e.g. by making sure the flow that
>> doesn't have an action has a match, but the one that has an action, doesn't
>> have a match. In this case we will end up with overlapping flows in the
>> detapath. These are fine as long as the wider one has all the actions.
>> However, this would need a very careful consideration, as it's hard to tell
>> right
>> away if the set of flows will be correct in the end or not.
>>
>> Let's see what will happen with a basic flow that changes 2
>> fields:
>>
>> ip, actions:set_field:A->eth_src,set_field:B->eth_dst
>>
>> Packet eth(A,C) results in datapath flow:
>> 1. eth(src=A), actions:set(eth(dst=B))
>>
>> Packet eth(C,B) results in datapath flow:
>> 2. eth(dst=B), actions:set(eth(src=A))
>>
>> Packet eth(A,B) results in datapath flow:
>> 3. eth(src=A,dst=B), actions:<none>
>>
>> Packet eth(C,D) results in datapath flow:
>> 4. <none>, actions:set(eth(src=A,dst=B))
>>
>> After all the flows installed:
>> Packet:
>> eth(A,C) -> matches #1 -> set(eth(dst=B)) -> eth(A,B)
>> matches #4 -> set(eth(src=A,dst=B)) -> eth(A,B)
>>
>> eth(A,B) -> matches all 4, ether way result will be eth(A,B)
>>
>> eth(C,B) -> matches #2 -> set(eth(src=A)) -> eth(A,B)
>> matches #4 -> set(eth(src=A,dst=B)) -> eth(A,B)
>>
>> eth(C,D) -> matches #4 -> set(eth(src=A,dst=B)) -> eth(A,B)
>>
>> So, this might work, assuming datapath correctly handles overlapping flows,
>> which it should. But it also becomes a bit more complex to process and adds
>> a
>> potential unnecessary overwrite of the fields that don't need it. The action
>> execution performance is likely not a problem here. I'm not sure if having
>> too
>> many overlapping flows may cause performance issues for the datapath's
>> packet classifier though. As seen above, we can have a lot of them with
>> increasing number of fields.
>>
>> The action commit code also changed quite a bit since the logic was
>> introduced, so there might be problems with this approach that I'm not aware
>> of.
>>
>> Best regards, Ilya Maximets.
>
> Hi Ilya,
>
> Thanks for your input.
> The optimization to avoid rewrite of the same value was done at [1]. Before
> that #3 dp-flow created in your example was like this:
> eth(src=A,dst=B), actions:set(eth(src=A,dst=B))
Not really. Commit [1] is only responsible for different fields within
the same header. However, if the header didn't change at all the actions
will not be generated even without it.
The logic goes back to [2] in 2011 where comparison of the base flow with
a new version of the flow was introduced. Also introducing a bug fixed
in 2013 in commit [3] by adding matches on all the fieleds for each we had
a set-style action.
>
> The question is what if we remove the match, but keep the actions, even if
> the value is the same.
> It means only #4 rule will be created.
> The downside is that we will do the rewrite even if not needed (same values),
> but on the other hand we decrease the number of dp-flows, consolidating them
> to match more flows.
> Can you point any functional issues with this approach?
If we only have a rule #4, that should be fine as it is has all the
actions. But I'm not really sure what is a good way to implement this,
since commit_odp_actions() only knows the old and a new state of the
flow, but doesn't know for which fields the set() actions were present
during the flow translation.
>
> Thanks,
> Eli
>
> [1] dbf4a92800d0 ("odp-util: Do not rewrite fields with the same values as
> matched")
[2] b3e9b2eda9ae ("ofproto: Optimize datapath actions.")
[3] f74e7df7450d ("ofproto-dpif: Always un-wildcard fields that are being set.")
_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
