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

[Eelco Chaudron]

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?

>> +                compose_ct_clear_action(ctx);
>> +                ctx->conntrack_ct_clear = true;
>> +            }
>>              break;
>>
>>          case OFPACT_NAT:
>> @@ -7514,6 +7520,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>>
>>          .was_mpls = false,
>>          .conntracked = false,
>> +        .conntrack_ct_clear = false,
>>
>>          .ct_nat_action = NULL,
>>
>> @@ -7577,6 +7584,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>>          if (!state->conntracked) {
>>              clear_conntrack(&ctx);
>>          }
>> +        ctx.conntrack_ct_clear = false;
>>
>>          /* Restore pipeline metadata. May change flow's in_port and other
>>           * metadata to the values that existed when freezing was triggered. */
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index 24bbd884c..e4702ece3 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -10842,6 +10842,40 @@ dnl
>>  NXT_PACKET_IN (xid=0x0): table_id=1 cookie=0x0 total_len=106 in_port=2 (via action) data_len=106 (unbuffered)
>>  udp,vlan_tci=0x0000,dl_src=50:54:00:00:00:0a,dl_dst=50:54:00:00:00:09,nw_src=10.1.1.2,nw_dst=10.1.1.1,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=2,tp_dst=1 udp_csum:553
>>  ])
>> +
>> +dnl The next test verifies that ct_clear at the datapath gets optimized if
>> +dnl executed in sequence.
>> +AT_DATA([flows.txt], [dnl
>> +table=0 in_port=1 actions=ct_clear,ct_clear,ct_clear,p2
>> +])
>> +AT_CHECK([ovs-ofctl del-flows br0])
>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=p1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2'], [0], [stdout])
>> +AT_CHECK([tail -1 stdout], [0],
>> +  [Datapath actions: ct_clear,2
>> +])
>> +AT_DATA([flows.txt], [dnl
>> +table=0 in_port=1 actions=ct_clear,goto_table:1
>> +table=1 in_port=1 actions=ct_clear,p2
>> +])
>> +AT_CHECK([ovs-ofctl del-flows br0])
>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=p1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2'], [0], [stdout])
>> +AT_CHECK([tail -1 stdout], [0],
>> +  [Datapath actions: ct_clear,2
>> +])
>> +AT_DATA([flows.txt], [dnl
>> +table=0 in_port=1 ip actions=ct_clear,ct(table=1)
>> +table=1 in_port=1 actions=ct_clear,p2
>> +])
>> +AT_CHECK([ovs-ofctl del-flows br0])
>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>> +AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=p1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=192.168.0.2'], [0], [stdout])
>> +AT_CHECK([grep Datapath stdout | sed 's/recirc(.*)/recirc(X)/'], [0],
>> +  [Datapath actions: ct_clear,ct,recirc(X)
>> +Datapath actions: ct_clear,2
>> +])
>> +
>>  OVS_VSWITCHD_STOP
>>  AT_CLEANUP
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>