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