[ovs-dev] [PATCH] dpctl: Properly reflect a rule's offloaded to HW state

Gavi Teitz gavi at mellanox.com
Sun Jun 10 15:05:39 UTC 2018


From: Simon Horman, Sent: Thursday, June 7, 2018 3:13 PM
>On Thu, Jun 07, 2018 at 09:36:59AM +0300, Gavi Teitz wrote:
>> Previously, any rule that is offloaded via a netdev, not necessarily 
>> to the HW, would be reported as "offloaded". This patch fixes this 
>> misalignment, and introduces the 'dp' state, as follows:
>> 
>> rule is in HW via TC offload  -> offloaded=yes dp:tc rule is in not HW 
>> over TC DP  -> offloaded=no  dp:tc rule is in not HW over OVS DP -> 
>> offloaded=no  dp:ovs
>> 
>> To achieve this, the flows's 'offloaded' flag was encapsulated in a 
>> new attrs struct, which contains the offloaded state of the flow and 
>> the DP layer the flow is handled in, and instead of setting the flow's 
>> 'offloaded' state based solely on the type of dump it was acquired 
>> via, for netdev flows it now sends the new attrs struct to be 
>> collected along with the rest of the flow via the netdev, allowing it 
>> to be set per flow.
>> 
>> For TC offloads, the offloaded state is set based on the 'in_hw' and 
>> 'not_in_hw' flags received from the TC as part of the flower. If no 
>> such flag was received, due to lack of kernel support, it defaults to 
>> true.
>
>Thanks for your patch, this seems quite nice.
>
>> 
>> Signed-off-by: Gavi Teitz <gavi at mellanox.com>
>> Acked-by: Roi Dayan <roid at mellanox.com>
>
>...
>
>> diff --git a/lib/dpctl.man b/lib/dpctl.man index 50623c4..7cf2087 
>> 100644
>> --- a/lib/dpctl.man
>> +++ b/lib/dpctl.man
>> @@ -119,9 +119,9 @@ flow. As an example, \fBfilter='tcp,tp_src=100'\fR 
>> will match the  datapath flow containing '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'.
>>  .IP
>>  If \fBtype=\fItype\fR is specified, only displays flows of a specific type.
>> -\fItype\fR can be \fBoffloaded\fR to display only offloaded rules or 
>> \fBOVS\fR -to display only non-offloaded rules.
>> -By default both offloaded and non-offloaded rules are displayed.
>> +\fItype\fR can be \fBoffloaded\fR to display only rules offloaded to 
>> +the HW or \fBOVS\fR to display only rules from the OVS tables.
>> +By default all rules are displayed.
>
>This patch allows differentiation of flows based on the datapath (ovs or tc) and offload (yes or no). But it seems that only the second is provided as a filter by dpctl via the type described above. Perhaps it would be useful to also expose the former?
>

This is something we would like to add, though we want to do it in a later commit so as not to delay the integration of this change. Also, I think that since we now can differentiate between flows based on the offloaded/not offloaded state and based on the datapath (ovs/tc/future implementation of a different datapath), we should be changing the current filter, which mixes the two, and have two filters, one of the offloaded state, and one of the datapath. Do you think that doing so would compromise backwards compatibility?

>>  .
>>  .IP "\*(DX\fBadd\-flow\fR [\fIdp\fR] \fIflow actions\fR"
>>  .TP
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 
>> 1bd10a6..aa9bbd9 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -1463,13 +1463,13 @@ dpif_netlink_init_flow_del(struct dpif_netlink 
>> *dpif,  }
>>  
>>  enum {
>> -    DUMP_OVS_FLOWS_BIT       = 0,
>> -    DUMP_OFFLOADED_FLOWS_BIT = 1,
>> +    DUMP_OVS_FLOWS_BIT    = 0,
>> +    DUMP_NETDEV_FLOWS_BIT = 1,
>>  };
>>  
>>  enum {
>> -    DUMP_OVS_FLOWS       = (1 << DUMP_OVS_FLOWS_BIT),
>> -    DUMP_OFFLOADED_FLOWS = (1 << DUMP_OFFLOADED_FLOWS_BIT),
>> +    DUMP_OVS_FLOWS    = (1 << DUMP_OVS_FLOWS_BIT),
>> +    DUMP_NETDEV_FLOWS = (1 << DUMP_NETDEV_FLOWS_BIT),
>
>Would TC make more sense than NETDEV here?

Probably not, since TC isn't a known name in this context, but rather the offload channel is generically named netdev.

>
>>  };
>>  
>>  struct dpif_netlink_flow_dump {
>
>...
>



More information about the dev mailing list