[ovs-dev,V2,1/1] dpif-netlink: Log eth type 0x1234 not offloadable
diff mbox series

Message ID 20190703045806.5357-1-elibr@mellanox.com
State Superseded
Headers show
Series
  • [ovs-dev,V2,1/1] dpif-netlink: Log eth type 0x1234 not offloadable
Related show

Commit Message

Eli Britstein July 3, 2019, 4:58 a.m. UTC
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@mellanox.com>
Acked-by: Roi Dayan <roid@mellanox.com>
Signed-off-by: Eli Britstein <elibr@mellanox.com>
---
 lib/dpif-netlink.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Ben Pfaff July 3, 2019, 6:11 p.m. UTC | #1
On Wed, Jul 03, 2019 at 04:58:06AM +0000, 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@mellanox.com>
> Acked-by: Roi Dayan <roid@mellanox.com>
> Signed-off-by: Eli Britstein <elibr@mellanox.com>

Acked-by: Ben Pfaff <blp@ovn.org>
Roi Dayan July 11, 2019, 1:11 p.m. UTC | #2
On 2019-07-03 9:11 PM, Ben Pfaff wrote:
> On Wed, Jul 03, 2019 at 04:58:06AM +0000, 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@mellanox.com>
>> Acked-by: Roi Dayan <roid@mellanox.com>
>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> 
> Acked-by: Ben Pfaff <blp@ovn.org>
> 

ping. can we merge this? 
thanks
Eli Britstein July 29, 2019, 5:22 p.m. UTC | #3
ping

On 7/11/2019 4:11 PM, Roi Dayan wrote:
>
> On 2019-07-03 9:11 PM, Ben Pfaff wrote:
>> On Wed, Jul 03, 2019 at 04:58:06AM +0000, 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@mellanox.com>
>>> Acked-by: Roi Dayan <roid@mellanox.com>
>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>> Acked-by: Ben Pfaff <blp@ovn.org>
>>
> ping. can we merge this?
> thanks
Ilya Maximets July 30, 2019, 10:10 a.m. UTC | #4
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@mellanox.com>
> Acked-by: Roi Dayan <roid@mellanox.com>
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Acked-by: Ben Pfaff <blp@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?

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?

Best regards, Ilya Maximets.
Eli Britstein July 30, 2019, 10:23 a.m. UTC | #5
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@mellanox.com>
>> Acked-by: Roi Dayan <roid@mellanox.com>
>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>> Acked-by: Ben Pfaff <blp@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.
>
> 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.
Ilya Maximets July 30, 2019, 10:30 a.m. UTC | #6
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@mellanox.com>
>>> Acked-by: Roi Dayan <roid@mellanox.com>
>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>>> Acked-by: Ben Pfaff <blp@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.

>>
>> 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.
Ilya Maximets July 30, 2019, 12:19 p.m. UTC | #7
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@mellanox.com>
>>>> Acked-by: Roi Dayan <roid@mellanox.com>
>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>>>> Acked-by: Ben Pfaff <blp@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.

> 
>>>
>>> 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.
Paul Blakey July 30, 2019, 1:50 p.m. UTC | #8
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@mellanox.com>
>>>>> Acked-by: Roi Dayan <roid@mellanox.com>
>>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>>>>> Acked-by: Ben Pfaff <blp@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.

>>>> 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.
Ilya Maximets July 30, 2019, 3:08 p.m. UTC | #9
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@mellanox.com>
>>>>>> Acked-by: Roi Dayan <roid@mellanox.com>
>>>>>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>>>>>> Acked-by: Ben Pfaff <blp@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.

Patch
diff mbox series

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