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 <[email protected]>
>>>>> Acked-by: Roi Dayan <[email protected]>
>>>>> Signed-off-by: Eli Britstein <[email protected]>
>>>>> Acked-by: Ben Pfaff <[email protected]>
>>>>> ---
>>>>> 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.
>>>> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
