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 <[email protected]> >>> --- >>> 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. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
