[ovs-dev] [PATCH ovs V8 01/26] tc: Add tc flower interface

[Simon Horman]

On Sun, May 07, 2017 at 02:46:14PM +0300, Roi Dayan wrote:
> 
> 
> On 04/05/2017 19:35, Simon Horman wrote:
> >On Wed, May 03, 2017 at 06:07:52PM +0300, Roi Dayan wrote:
> >>From: Paul Blakey <paulb at mellanox.com>
> >>
> >>Add tc flower interface that will be used to offload flows via tc
> >>flower classifier. Depending on the flag used (skip_sw/hw) flower
> >>will pass those to HW or handle them itself.
> >>Move some tc related functions from netdev-linux.c to tc.c
> >>
> >>Co-authored-by: Shahar Klein <shahark at mellanox.com>
> >>Signed-off-by: Shahar Klein <shahark at mellanox.com>
> >>Signed-off-by: Paul Blakey <paulb at mellanox.com>
> >>Reviewed-by: Roi Dayan <roid at mellanox.com>
> >>Reviewed-by: Simon Horman <simon.horman at netronome.com>
> >>---
> >> lib/automake.mk    |    2 +
> >> lib/netdev-linux.c |  164 ++------
> >> lib/tc.c           | 1109 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> lib/tc.h           |  128 ++++++
> >> 4 files changed, 1279 insertions(+), 124 deletions(-)
> >> create mode 100644 lib/tc.c
> >> create mode 100644 lib/tc.h
> >>
> >>diff --git a/lib/automake.mk b/lib/automake.mk
> >>index faace79..3d57610 100644
> >>--- a/lib/automake.mk
> >>+++ b/lib/automake.mk
> >>@@ -352,6 +352,8 @@ if LINUX
> >> lib_libopenvswitch_la_SOURCES += \
> >> 	lib/dpif-netlink.c \
> >> 	lib/dpif-netlink.h \
> >>+	lib/tc.h \
> >>+	lib/tc.c \
> >
> >tc.c seems to contain two types of functions:
> >
> >1. Code which is used by both (old) netdev-linux.c paths and
> >   code which is used by (new) tc-flower specific paths.
> >   For example tc_transact().
> >2. Code which is specific to tc-flower
> >
> >The latter does not compile against old kernel headers.
> >
> >As per Flavio Leitner's review or v7 it seems that the compilation problem
> >may be addressed by patch 23.
> 
> this is correct. we did first all work for hw offload and then added a
> compat fix commit.
> Isn't it ok since there is no point for half work for hw offload?

Its not ok because this patch does not compile which breaks bisection.

It may be that Flavio's suggestion is not the best way to resolve the
problem - another idea I have is to conditionally compile the tc_flower.c
that I suggest below and provide stub functions in tc_flower.h for the case
where tc_flower.c is not compiled.

> >I think it would also be worth considering splitting the TC code such that
> >tc-flower specific code to is present in tc_flower.[ch] and leave shared
> >code is in tc.[ch].
> >
> >Moving code to tc.[ch] could be a separate patch to adding tc_flower.[ch].
> >In my opinion smaller patches are easier to review and possibly merge
> >incrementally.
> 
> I agree that first commit should do only the moving and second to add new
> code but most of the functions are flower related. I'm not sure how much
> will stay in tc.c after removing flower related code to a new file.

Thanks, I think that would make the patches rather a lot easier on the
eyes.

...

Thanks for your responses to the other, more specific, review comments.