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

Ilya Maximets i.maximets at ovn.org
Tue Oct 20 15:29:37 UTC 2020


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.

> 
> 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