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 ?
> + 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
