[ovs-dev] [PATCH branch-2.13] netdev-offload-dpdk: Support vxlan encap offload with load actions

Sriharsha Basavapatna sriharsha.basavapatna at broadcom.com
Tue Oct 20 19:08:29 UTC 2020


On Tue, Oct 20, 2020 at 8:59 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> On 10/20/20 5:04 PM, Eli Britstein wrote:
> >
> > On 10/20/2020 1:12 PM, Sriharsha Basavapatna wrote:
> >> On Tue, Oct 20, 2020 at 2:40 PM Ilya Maximets <i.maximets at ovn.org> wrote:
> >>> On 10/20/20 10:51 AM, Sriharsha Basavapatna wrote:
> >>>> On Mon, Oct 19, 2020 at 7:59 PM Ilya Maximets <i.maximets at ovn.org> wrote:
> >>>>> On 10/18/20 9:10 AM, Eli Britstein wrote:
> >>>>>> Struct match has the tunnel values/masks in
> >>>>>> match->flow.tunnel/match->wc.masks.tunnel.
> >>>>>> Load actions such as load:0xa566c10->NXM_NX_TUN_IPV4_DST[],
> >>>>>> load:0xbba->NXM_NX_TUN_ID[] are utilizing the tunnel masks fields, but
> >>>>>> those should not be used for matching.
> >>>>>> Offloading fails if masks is not clear. Fix it by checking if tunnel is
> >>>>>> present.
> >>>>>>
> >>>>>> Signed-off-by: Eli Britstein <elibr at nvidia.com>
> >>>>>> ---
> >>>>> Thanks, Eli.
> >>>>>
> >>>>> Harsha, Emma, could you, please, review/test this version?
> >>>>>
> >>>>> Best regards, Ilya Maximets.
> >>>> Reviewed this change. Do we even need this backported to 2.13, since
> >>>> we don't support clone/tnl-push actions with offload ?
> >>> I think we could have metadata partially set even if we're not performing
> >>> clone/tnl-push actions.  Maybe something like this:
> >>>
> >>>    table=0,in_port=1,ip,actions=load:0xbba->NXM_NX_TUN_ID[],goto_table(1)
> >>>    table=1,ip,nw_src=192.168.0.1,actions=load:0xa566c10->NXM_NX_TUN_IPV4_DST[],<out to tunnel>
> >>>    table=1,ip,nw_src=192.168.0.2,actions=drop
> >>>
> >>> In this scenario packet that goes to 'drop' action might have tun_id set
> >>> in the metadata and we will not offload it.  I didn't test that though.
> >>> Does that make sense?
> >> I tested with the above set of rules. It doesn't fail in
> >> validate_flow() even without this fix, since it checks
> >> match_zero_wc.flow.tunnel and not match_zero_wc.masks.tunnel which has
> >> the tun_id (mask) set. So, this fix doesn't make any difference
> >> (validate succeeds with or without it).
> >>
> >> (gdb) p /x match_zero_wc.flow.tunnel.tun_id
> >> $4 = 0x0
> >> (gdb) p /x match_zero_wc.wc.masks.tunnel.tun_id
> >> $5 = 0xffffffffffffffff
> >
> > Harsha - you are right. Thanks. This patch is not needed there as is. Maybe it was intentional to check the "flow" and not "masks" for tunnel, so I think we can abandon it for backport <=2.13.
>
> Good catch. Thanks, Harsha!
>
> Lets abandon this patch in this case.

Thanks Ilya and Eli.
-Harsha

>
> >
> > Regarding not supporting clone/tnl-push - it is not related. The issue is (might have been) with validation of matches. If we checked the masks we would fail also the partial offload.
> >
> >>
> >> Thanks,
> >> -Harsha
> >>>> Thanks,
> >>>> -Harsha
> >>>>>>   lib/netdev-offload-dpdk.c | 3 ++-
> >>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> >>>>>> index 4538baf5e..c68d539ea 100644
> >>>>>> --- a/lib/netdev-offload-dpdk.c
> >>>>>> +++ b/lib/netdev-offload-dpdk.c
> >>>>>> @@ -1092,7 +1092,8 @@ netdev_offload_dpdk_validate_flow(const struct match *match)
> >>>>>>       /* Create a wc-zeroed version of flow. */
> >>>>>>       match_init(&match_zero_wc, &match->flow, &match->wc);
> >>>>>>
> >>>>>> -    if (!is_all_zeros(&match_zero_wc.flow.tunnel,
> >>>>>> +    if (flow_tnl_dst_is_set(&match->flow.tunnel) &&
> >>>>>> +        !is_all_zeros(&match_zero_wc.flow.tunnel,
> >>>>>>                         sizeof match_zero_wc.flow.tunnel)) {
> >>>>>>           goto err;
> >>>>>>       }
> >>>>>>
>


More information about the dev mailing list