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

Eli Britstein elibr at nvidia.com
Tue Oct 20 15:04:47 UTC 2020


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.

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