[ovs-dev] [PATCH] ofproto-dpif-xlate: avoid successive ct_clear datapath actions

[Eelco Chaudron]

On 14 May 2021, at 12:01, Ilya Maximets wrote:

> On 5/14/21 11:42 AM, Eelco Chaudron wrote:
>>
>>
>> On 14 May 2021, at 11:31, Ilya Maximets wrote:
>>
>>> On 5/14/21 11:17 AM, Eelco Chaudron wrote:
>>>> Due to flow lookup optimizations, especially in the resubmit/clone cases,
>>>> we might end up with multiple ct_clear actions, which are not necessary.
>>>>
>>>> This patch avoids adding the successive ct_clear actions in the datapath.
>>>>
>>>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>>>> ---
>>>>  ofproto/ofproto-dpif-xlate.c |   10 +++++++++-
>>>>  tests/ofproto-dpif.at        |   34 ++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 43 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>>> index 7108c8a30..b3eb0a9c1 100644
>>>> --- a/ofproto/ofproto-dpif-xlate.c
>>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>>> @@ -396,6 +396,9 @@ struct xlate_ctx {
>>>>       * state from the datapath should be honored after thawing. */
>>>>      bool conntracked;
>>>>
>>>> +    /* True if the current action set processed contains a ct_clear action. */
>>>> +    bool conntrack_ct_clear;
>>>> +
>>>>      /* Pointer to an embedded NAT action in a conntrack action, or NULL. */
>>>>      struct ofpact_nat *ct_nat_action;
>>>>
>>>> @@ -7127,7 +7130,10 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>>>>              break;
>>>>
>>>>          case OFPACT_CT_CLEAR:
>>>> -            compose_ct_clear_action(ctx);
>>>> +            if (!ctx->conntrack_ct_clear) {
>>>
>>> Doesn't ctx->contracked hold the information that conntrack
>>> is clear or not?   Can we just avoid clearing conntrack if
>>> it's already clear, i.e. !ctx->conntracked ?
>>
>> Yes, I thought about that, but it would suppress the ct_clear in the datapath altogether if no conntrack information is present. This might confuse people looking at the datapath dumps, or do you think this should be ok?
>
> It should be understandable from the datapath pipeline that there
> is no conntrack state.
>
> I think, there is no point to have a datapath action that does nothing.
> It only hurts performance.  We need to be sure that OVS constructs
> correct matches, though.  It should, but we, probabaly, need a test.
> I don't see any existing tests that checks datapath flows with ct_clear
> and this patch adds only very basic ones.  And I think that 2 of 3 tests
> in this patch will not have any ct_clear action if we'll check for
> !ctx->conntracked.

Ack, will rework the patch to use contracted instead, and update the respective tests.