On 10/16/20 11:38 AM, Ilya Maximets wrote:
> On 10/16/20 7:57 AM, Eli Britstein wrote:
>>
>> On 10/16/2020 3:02 AM, Ilya Maximets wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 10/12/20 5:25 PM, Sriharsha Basavapatna via dev wrote:
>>>> On Mon, Oct 12, 2020 at 4:48 PM Eli Britstein <[email protected]> wrote:
>>>>>
>>>>> On 10/12/2020 10:20 AM, Sriharsha Basavapatna wrote:
>>>>>> On Sun, Sep 6, 2020 at 5:51 PM Eli Britstein <[email protected]> wrote:
>>>>>>> ping
>>>>>>>
>>>>>>> On 7/30/2020 1:58 PM, Eli Britstein wrote:
>>>>>>>> From: Lei Wang <[email protected]>
>>>>>>>>
>>>>>>>> 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. Clear it if no tunnel used.
>>>>>>>>
>>>>>>>> Signed-off-by: Lei Wang <[email protected]>
>>>>>>>> Reviewed-by: Eli Britstein <[email protected]>
>>>>>>>> Reviewed-by: Gaetan Rivet <[email protected]>
>>>>>>>> Signed-off-by: Eli Britstein <[email protected]>
>>>> Acked-by: Sriharsha Basavapatna <[email protected]>
>>>>
>>>> See my comment below.
>>>>
>>>>>>>> ---
>>>>>>>> lib/netdev-offload-dpdk.c | 4 ++++
>>>>>>>> 1 file changed, 4 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>>>>>>> index de6101e4d..0d23e4879 100644
>>>>>>>> --- a/lib/netdev-offload-dpdk.c
>>>>>>>> +++ b/lib/netdev-offload-dpdk.c
>>>>>>>> @@ -682,6 +682,10 @@ parse_flow_match(struct flow_patterns *patterns,
>>>>>>>>
>>>>>>>> consumed_masks = &match->wc.masks;
>>>>>>>>
>>>>>>>> + if (!flow_tnl_dst_is_set(&match->flow.tunnel)) {
>>>>>>>> + memset(&match->wc.masks.tunnel, 0, sizeof
>>>>>>>> match->wc.masks.tunnel);
>>> Small nit: For consistency, I think, we should access via 'consumed_masks'
>>> pointer, not directly. What do you think?
>>> I could change that on commit.
>> OK.
>>>
>>>
>>> Another point is that we could actually consider this as a bug fix, but
>>> I'm not sure which commit to use for a 'Fixes' tag. Suggestions?
>>> Technically, the issue exists on all branches down to 2.10, but 2.13 and
>>> lower will require a bit different fix due t major rework of this code.
>>
>> I see the original "validate" code also has it, so maybe e8a2b5bf92bb
>> ("netdev-dpdk: implement flow offload with rte flow")
>>
>> Do you want me to send v2 with this fixes tag and usage of consumed_masks
>> (above), or you can do it when applying?
>
> I can do that.
> I will be good if you can prepare fix for branch-2.13, i.e. for the
*it
> original "validate" code.
>
>>
>> Thanks, Eli
>>
>>>
>>>>>>>> + }
>>>>>>>> +
>>>>>> This fix looks good to me. Did you take a look to see if this can be
>>>>>> fixed in the code that generates these invalid masks in the first
>>>>>> place ?
>>>>> I wouldn't say it's "invalid masks". OVS takes those masks into
>>>>> consideration with such usage of flow_tnl_dst_is_set, that is done in
>>>>> several places in the code. For example odp-util.c,
>>>>> lib/netdev-offload-tc.c.
>>>> It is invalid because the datapath rule is specifying tunnel masks
>>>> when we are not really matching on them. In the context of encap
>>>> actions (which is what this patch is fixing), tunnel masks shouldn't
>>>> be set.
>>> It seems like tunnel metadata and a couple of other fields are special
>>> cases that allowed to have masks set without having keys. Datapaths
>>> handles these cases. This was done for some reason, but it hard to
>>> track down why exactly. Following commit made higher layers to always
>>> provide tunnel masks even if tunnel keys are not specified:
>>> 01fcdfc6b322 ("odp-util: Always serialize tunnel mask attributes.")
>>> It looks like it was intended to simplify processing, but I'm not sure.
>>> Anyway, datapath should not look at these masks if tnl_dst is not
>>> specified in keys as this is a sign of a correct tunnel metadata. So,
>>> the fix looks correct.
>>>
>>>>>> Thanks,
>>>>>> -Harsha
>>>>>>>> memset(&consumed_masks->in_port, 0, sizeof
>>>>>>>> consumed_masks->in_port);
>>>>>>>> /* recirc id must be zero. */
>>>>>>>> if (match->wc.masks.recirc_id & match->flow.recirc_id) {
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
