On 6/3/22 17:31, Rosemarie O'Riorden wrote:
> When OVS sees a tunnel push with a nested list next, it will not
> clone the packet, as a clone is not needed. However, a clone action will
> still be created with the tunnel push encapulated inside. There is no
> need to create the clone action in this case, as extra parsing will need
> to be performed, which is less efficient.
>
> Signed-off-by: Rosemarie O'Riorden <[email protected]>
> ---
> v2:
> - Fixes tests that are affected by the change
> v3:
> - Adds support for hardware offloading
> v4:
> - Adds negative check in test
> - Fixes logic in function native_tunnel_output
> - Stylistic and organizational improvements
>
> lib/netdev-offload-dpdk.c | 38 +++++++++-----
> lib/netlink.c | 7 +++
> lib/netlink.h | 1 +
> ofproto/ofproto-dpif-xlate.c | 22 ++++++--
> tests/nsh.at | 6 +--
> tests/packet-type-aware.at | 24 ++++-----
> tests/tunnel-push-pop-ipv6.at | 20 +++----
> tests/tunnel-push-pop.at | 98 ++++++++++++++++++++++++++++-------
> 8 files changed, 152 insertions(+), 64 deletions(-)
>
<snip>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9ea21edc4..a908160a5 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3739,7 +3739,11 @@ native_tunnel_output(struct xlate_ctx *ctx, const
> struct xport *xport,
> size_t clone_ofs = 0;
> size_t push_action_size;
>
> - clone_ofs = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
> + if (!is_last_action) {
> + clone_ofs = nl_msg_start_nested(ctx->odp_actions,
> + OVS_ACTION_ATTR_CLONE);
> + }
> + size_t pre_patch_port_outp_size = ctx->odp_actions->size;
Hi, Rosemarie. Thanks for addressing previous comments!
This version looks correct to me. I also did some tests
with dummy offload and they seem to work fine.
The only thing I think we can improve is the variable name
here. I think, it will be a bit cleaner if we will rename
and re-use the same variable for the offset in both cases.
Maybe something like this:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0c92ed922..c645c3470 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3736,14 +3736,12 @@ native_tunnel_output(struct xlate_ctx *ctx, const
struct xport *xport,
s_ip, tnl_params.is_ipv6,
tnl_push_data.tnl_type);
- size_t clone_ofs = 0;
+ size_t offset;
size_t push_action_size;
- if (!is_last_action) {
- clone_ofs = nl_msg_start_nested(ctx->odp_actions,
- OVS_ACTION_ATTR_CLONE);
- }
- size_t pre_patch_port_outp_size = ctx->odp_actions->size;
+ offset = is_last_action
+ ? ctx->odp_actions->size
+ : nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_CLONE);
odp_put_tnl_push_action(ctx->odp_actions, &tnl_push_data);
push_action_size = ctx->odp_actions->size;
---
What do you think?
If that looks good to you, I can just apply the diff above and
rename the variable in other places while applying the change.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
