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.
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 <rorior...@redhat.com>
>> ---
>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
