On Tue, Jul 26, 2022 at 11:40 AM Eelco Chaudron <[email protected]> wrote:
> When OFPROTO non-reversible actions are translated to data plane
> actions, the only thing looked at is if there are more actions
> pending. If this is the case, the action is encapsulated in a
> clone().
>
> This could lead to unnecessary clones if no meaningful data
> plane actions are added. For example, the register pop in the
> included test case.
>
> The best solution would probably be to build the full action
> path and determine if the clone is needed. However, this would
> be a huge change in the existing design, so for now, we just try
> to optimize the generated datapath flow. We can revisit this
> later, as some of the pending CT issues might need this rework.
>
> Fixes: feee58b9587f ("ofproto-dpif-xlate: Keep track of the last action")
> Fixes: dadd8357f224 ("ofproto-dpif: Fix issue with non-reversible actions
> on a patch ports.")
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
> ofproto/ofproto-dpif-xlate.c | 37
> +++++++++++++++++++++++++++++++++++++
> tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++
> tests/system-layer3-tunnels.at | 2 +-
> tests/tunnel-push-pop.at | 4 ++--
> 4 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index fda802e83..06fe47f6a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7667,6 +7667,39 @@ xlate_wc_finish(struct xlate_ctx *ctx)
> }
> }
>
> +/* This will optimize the odp actions generated. For now, it will remove
> + * trailing clone actions that are unnecessary. */
> +static void
> +xlate_optimize_odp_actions(struct xlate_in *xin)
> +{
> + struct ofpbuf *actions = xin->odp_actions;
> + struct nlattr *last_action = NULL;
> + struct nlattr *a;
> + int left;
> +
> + if (!actions) {
> + return;
> + }
> +
> + /* Find the last action in the set. */
> + NL_ATTR_FOR_EACH (a, left, actions->data, actions->size) {
> + last_action = a;
> + }
> +
> + /* Remove the trailing clone() action, by directly embedding the
> nested
> + * actions. */
> + if (last_action && nl_attr_type(last_action) ==
> OVS_ACTION_ATTR_CLONE) {
> + void *dest;
> +
> + nl_msg_reset_size(actions,
> + (unsigned char *) last_action -
> + (unsigned char *) actions->data);
> +
> + dest = nl_msg_put_uninit(actions, nl_attr_get_size(last_action));
> + memmove(dest, nl_attr_get(last_action),
> nl_attr_get_size(last_action));
> + }
> +}
> +
> /* Translates the flow, actions, or rule in 'xin' into datapath actions in
> * 'xout'.
> * The caller must take responsibility for eventually freeing 'xout', with
> @@ -8092,6 +8125,10 @@ exit:
> if (xin->odp_actions) {
> ofpbuf_clear(xin->odp_actions);
> }
> + } else {
> + /* In the non-error case, see if we can further optimize the
> datapath
> + * rules by removing redundant (clone) actions. */
> + xlate_optimize_odp_actions(xin);
> }
>
> /* Install drop action if datapath supports explicit drop action. */
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 2e91ae1a1..b91166e88 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -8923,6 +8923,40 @@ AT_CHECK([tail -1 stdout], [0],
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([ofproto-dpif - patch ports - no additional clone])
> +OVS_VSWITCHD_START(
> + [add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 -- \
> + add-port br0 p1 -- set Interface p1 type=patch \
> + options:peer=p2 ofport_request=2
> -- \
> + add-br br1 -- \
> + set bridge br1 other-config:hwaddr=aa:66:aa:66:00:00 -- \
> + set bridge br1 datapath-type=dummy other-config:datapath-id=1234 \
> + fail-mode=secure -- \
> + add-port br1 p2 -- set Interface p2 type=patch \
> + options:peer=p1 -- \
> + add-port br1 p3 -- set Interface p3 type=dummy ofport_request=3])
> +
> +AT_DATA([flows-br0.txt], [dnl
>
> +priority=10,tcp,action=push:NXM_OF_IN_PORT[],resubmit(,65),pop:NXM_OF_IN_PORT[]
> +table=65,priority=10,ip,in_port=p0,action=p1
> +])
> +
> +AT_DATA([flows-br1.txt], [dnl
> +priority=100,in_port=p2,tcp,ct_state=-trk,action=ct(table=0,zone=1)
> +priority=100,in_port=p2,tcp,ct_state=+trk+est,ct_zone=1,action=p3
> +])
> +
> +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows-br0.txt])
> +AT_CHECK([ovs-ofctl --bundle add-flows br1 flows-br1.txt])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0 in_port=p0,tcp --ct-next 'trk,est'
> | \
> + grep "Datapath actions:" | grep -q clone],
> + [1], [], [],
> + [ovs-appctl ofproto/trace br0 in_port=p0,tcp --ct-next
> 'trk,est'])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> dnl ----------------------------------------------------------------------
> AT_BANNER([ofproto-dpif -- megaflows])
>
> diff --git a/tests/system-layer3-tunnels.at b/tests/
> system-layer3-tunnels.at
> index d21fd777d..c37852b21 100644
> --- a/tests/system-layer3-tunnels.at
> +++ b/tests/system-layer3-tunnels.at
> @@ -147,7 +147,7 @@ AT_CHECK([tail -1 stdout], [0],
> dnl Check GRE tunnel push
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(3),eth(dst=f9:bc:12:44:34:b6,src=af:55:aa:55:00:03),eth_type(0x0800),ipv4(src=1.1.3.88,dst=1.1.3.92,proto=1,tos=0,ttl=64,frag=no)'],
> [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
> - [Datapath actions:
> clone(tnl_push(tnl_port(4),header(size=38,type=3,eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:03,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x6558))),out_port(2)),1)
> + [Datapath actions:
> tnl_push(tnl_port(4),header(size=38,type=3,eth(dst=f8:bc:12:44:34:b6,src=aa:55:aa:55:00:03,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.92,proto=47,tos=0,ttl=64,frag=0x4000),gre((flags=0x0,proto=0x6558))),out_port(2)),1
> ])
>
> OVS_VSWITCHD_STOP
> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
> index 2c6c52eda..bc82abc1f 100644
> --- a/tests/tunnel-push-pop.at
> +++ b/tests/tunnel-push-pop.at
> @@ -906,11 +906,11 @@ AT_CHECK([ovs-appctl ofproto/trace ovs-tun0
> in_port=p7], [0], [stdout])
> AT_CHECK([tail -2 stdout], [0], [dnl
> Megaflow: recirc_id=0,eth,in_port=7,dl_src=00:00:00:00:00:00,dnl
> dl_dst=00:00:00:00:00:00,dl_type=0x0000
> -Datapath actions:
> push_vlan(vid=200,pcp=0),1,clone(tnl_push(tnl_port(4789),dnl
> +Datapath actions: push_vlan(vid=200,pcp=0),1,tnl_push(tnl_port(4789),dnl
> header(size=50,type=4,eth(dst=aa:55:aa:66:00:00,src=aa:55:aa:55:00:00,dnl
> dl_type=0x0800),ipv4(src=10.0.0.2,dst=10.0.0.11,proto=17,tos=0,ttl=64,dnl
>
> frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0x8000000,vni=0x0)),dnl
> -out_port(100)),8)
> +out_port(100)),8
> ])
>
> OVS_VSWITCHD_STOP
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Hi,
not sure how much is my review valuable, but:
Acked-by: Ales Musil <[email protected]>
Regards,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
[email protected] IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
