[ovs-dev] [PATCH V2 1/1] dpif-netlink: Log eth type 0x1234 not offloadable

[Ilya Maximets]

On 30.07.2019 16:50, Paul Blakey wrote:
> 
> On 7/30/2019 3:19 PM, Ilya Maximets wrote:
>> On 30.07.2019 13:30, Ilya Maximets wrote:
>>> On 30.07.2019 13:23, Eli Britstein wrote:
>>>> On 7/30/2019 1:10 PM, Ilya Maximets wrote:
>>>>> On 03.07.2019 7:58, Eli Britstein wrote:
>>>>>> Ethernet type 0x1234 is used for testing and not being offloadable. For
>>>>>> testing offloadable features, log about using this value.
>>>>>>
>>>>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>>>>> Acked-by: Roi Dayan <roid at mellanox.com>
>>>>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>>>>> Acked-by: Ben Pfaff <blp at ovn.org>
>>>>>> ---
>>>>>>    lib/dpif-netlink.c | 1 +
>>>>>>    1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>>>>> index ba80a0079..a0d51ae61 100644
>>>>>> --- a/lib/dpif-netlink.c
>>>>>> +++ b/lib/dpif-netlink.c
>>>>>> @@ -2007,6 +2007,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>>>>>>    
>>>>>>        /* When we try to install a dummy flow from a probed feature. */
>>>>>>        if (match.flow.dl_type == htons(0x1234)) {
>>>>>> +        VLOG_INFO_RL(&rl, "eth 0x1234 is special and not offloadable");
>>>>>>            return EOPNOTSUPP;
>>>>>>        }
>>>>> But what is the purpose of this patch?  What is the use case?
>>>> RH wanted to test that dl_type is offloaded. Coincidentally, they used
>>>> 0x1234, and it was not offloaded, and they didn't understand why, and
>>>> suggested a log message.
>>> I'll take a closer look, but it seems that we just need to remove this
>>> 'if' statement and allow oflloading.
>> 'dpif_probe_feature' always has DPIF_FP_PROBE flag set. Other probing code uses
>> dpif_execute() which uses DPIF_OP_EXECUTE, hence never calls parse_flow_put().
>> So, this 'if' statement is wrong and should be deleted as it only forbids
>> offloading of the real legitimate flows with dl_type 0x1234. Dummy flows never
>> reaches this code.
> 
> Couldn't find any reason for it, I even looked at diff from my commit 
> that introduced it.
> 
> Seems safe to remove it.

OK. Thanks, Paul. I'll prepare a patch.

> 
>>>>> Actually, it looks like we need to just remove above 'if' statement
>>>>> entirely.  Just a few lines above there is a check if we are probing
>>>>> or installing real flow:
>>>>>
>>>>>      if (put->flags & DPIF_FP_PROBE) {
>>>>>          return EOPNOTSUPP;
>>>>>      }
>>>>>
>>>>> So, we will never get there while probing.  But why we're restricting
>>>>> this flow type from being offloaded?  'netdev_flow_put' will refuse
>>>>> to offload if it doesn't know that flow type, but this shouldn't be
>>>>> done here.
>>>>>
>>>>> In case we have a dummy flow without DPIF_FP_PROBE flag set, we need
>>>>> to fix upper layers.  Is it the case?
>>>> I didn't look into it why we restrict it and if there is a real reason
>>>> why in this layer. You may be right, but I don't know.
>>>>
>>>>> Best regards, Ilya Maximets.