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

Simon Horman simon.horman at netronome.com
Mon Jun 18 09:30:39 UTC 2018


On Sun, Jun 10, 2018 at 03:05:39PM +0000, Gavi Teitz wrote:
> 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?

I think it should be possible to arrange things so that there is 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.

Understood.

I have gone ahead and applied the patch to master.
There was a minor conflict when applying the manpage hunk.
Please send an incremental patch to fix things if I got that wrong.


More information about the dev mailing list