On 6/29/22 18:28, Rosemarie O'Riorden wrote: > Hi Ilya, > > > Thank you for reviewing my patch and suggesting a fix. It looks great to > me. You can do as you said, and apply the changes to my patch.
Thanks for checking! Applied now. Best regards, Ilya Maximets. > > > Rosemarie O'Riorden > > On 6/28/22 07:06, Ilya Maximets wrote: > >> 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
