Re: [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 
> >>
> >>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 
> >>Signed-off-by: Shahar Klein 
> >>Signed-off-by: Paul Blakey 
> >>Reviewed-by: Roi Dayan 
> >>Reviewed-by: Simon Horman 
> >>---
> >> 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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[Roi Dayan]

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 

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 
Signed-off-by: Shahar Klein 
Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
Reviewed-by: Simon Horman 
---
 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?



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.




Overall this patch-set seems very large and I think it would be worth
considering ways to merge it incrementally.

...


diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 79e8273..a6bb515 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c


...


@@ -2094,7 +2095,7 @@ netdev_linux_set_policing(struct netdev *netdev_,

 COVERAGE_INC(netdev_set_policing);
 /* Remove any existing ingress qdisc. */
-error = tc_add_del_ingress_qdisc(netdev_, false);
+error = tc_add_del_ingress_qdisc(ifindex, false);


This patch both changes the signature of tc_add_del_ingress_qdisc() and
moves it to tc.c. The signature change could be in a separate patch.


ok



...


@@ -2930,8 +2931,8 @@ codel_setup_qdisc__(struct netdev *netdev, uint32_t 
target, uint32_t limit,

 tc_del_qdisc(netdev);

-tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
-NLM_F_EXCL | NLM_F_CREATE, );
+tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
+ NLM_F_EXCL | NLM_F_CREATE, );


Likewise, I think reworking tc_make_request() could be a separate patch.

...


@@ -4222,13 +4224,11 @@ hfsc_setup_qdisc__(struct netdev * netdev)

 tc_del_qdisc(netdev);

-tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
-NLM_F_EXCL | NLM_F_CREATE, );
-
+tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
+ NLM_F_EXCL | NLM_F_CREATE, );
 if (!tcmsg) {
 return ENODEV;
 }
-


The change above seems spurious.


 tcmsg->tcm_handle = tc_make_handle(1, 0);
 tcmsg->tcm_parent = TC_H_ROOT;

@@ -4255,12 +4255,11 @@ hfsc_setup_class__(struct netdev *netdev, unsigned int 
handle,
 struct ofpbuf request;
 struct tc_service_curve min, max;

-tcmsg = tc_make_request(netdev, RTM_NEWTCLASS, NLM_F_CREATE, );
-
+tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTCLASS,
+ NLM_F_CREATE, );
 if (!tcmsg) {
 return ENODEV;
 }
-


Ditto.


 tcmsg->tcm_handle = handle;
 tcmsg->tcm_parent = parent;



...


diff --git a/lib/tc.c b/lib/tc.c
new file mode 100644
index 000..cd06025
--- /dev/null
+++ b/lib/tc.c
@@ -0,0 +1,1109 @@


...


+static const struct nl_policy tca_flower_policy[] = {
+[TCA_FLOWER_CLASSID] = { .type = NL_A_U32, .optional = true, },
+[TCA_FLOWER_INDEV] = { .type = NL_A_STRING, .max_len = IFNAMSIZ,
+   .optional = true, },
+[TCA_FLOWER_KEY_ETH_SRC] = { .type = NL_A_UNSPEC,
+ .min_len = ETH_ALEN, .optional = true, },
+

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

[Simon Horman]

On Wed, May 03, 2017 at 06:07:52PM +0300, Roi Dayan wrote:
> From: Paul Blakey 
> 
> 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 
> Signed-off-by: Shahar Klein 
> Signed-off-by: Paul Blakey 
> Reviewed-by: Roi Dayan 
> Reviewed-by: Simon Horman 
> ---
>  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.

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.

Overall this patch-set seems very large and I think it would be worth
considering ways to merge it incrementally.

...

> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 79e8273..a6bb515 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c

...

> @@ -2094,7 +2095,7 @@ netdev_linux_set_policing(struct netdev *netdev_,
>  
>  COVERAGE_INC(netdev_set_policing);
>  /* Remove any existing ingress qdisc. */
> -error = tc_add_del_ingress_qdisc(netdev_, false);
> +error = tc_add_del_ingress_qdisc(ifindex, false);

This patch both changes the signature of tc_add_del_ingress_qdisc() and
moves it to tc.c. The signature change could be in a separate patch.

...

> @@ -2930,8 +2931,8 @@ codel_setup_qdisc__(struct netdev *netdev, uint32_t 
> target, uint32_t limit,
>  
>  tc_del_qdisc(netdev);
>  
> -tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
> -NLM_F_EXCL | NLM_F_CREATE, );
> +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
> + NLM_F_EXCL | NLM_F_CREATE, 
> );

Likewise, I think reworking tc_make_request() could be a separate patch.

...

> @@ -4222,13 +4224,11 @@ hfsc_setup_qdisc__(struct netdev * netdev)
>  
>  tc_del_qdisc(netdev);
>  
> -tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
> -NLM_F_EXCL | NLM_F_CREATE, );
> -
> +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
> + NLM_F_EXCL | NLM_F_CREATE, 
> );
>  if (!tcmsg) {
>  return ENODEV;
>  }
> -

The change above seems spurious.

>  tcmsg->tcm_handle = tc_make_handle(1, 0);
>  tcmsg->tcm_parent = TC_H_ROOT;
>  
> @@ -4255,12 +4255,11 @@ hfsc_setup_class__(struct netdev *netdev, unsigned 
> int handle,
>  struct ofpbuf request;
>  struct tc_service_curve min, max;
>  
> -tcmsg = tc_make_request(netdev, RTM_NEWTCLASS, NLM_F_CREATE, );
> -
> +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTCLASS,
> + NLM_F_CREATE, );
>  if (!tcmsg) {
>  return ENODEV;
>  }
> -

Ditto.

>  tcmsg->tcm_handle = handle;
>  tcmsg->tcm_parent = parent;
>  

...

> diff --git a/lib/tc.c b/lib/tc.c
> new file mode 100644
> index 000..cd06025
> --- /dev/null
> +++ b/lib/tc.c
> @@ -0,0 +1,1109 @@

...

> +static const struct nl_policy tca_flower_policy[] = {
> +[TCA_FLOWER_CLASSID] = { .type = NL_A_U32, .optional = true, },
> +[TCA_FLOWER_INDEV] = { .type = NL_A_STRING, .max_len = IFNAMSIZ,
> +   .optional = true, },
> +[TCA_FLOWER_KEY_ETH_SRC] = { .type = NL_A_UNSPEC,
> + .min_len = ETH_ALEN, .optional = true, },
> +[TCA_FLOWER_KEY_ETH_DST] = { .type = NL_A_UNSPEC,
> + .min_len = ETH_ALEN, .optional = true, },
> +[TCA_FLOWER_KEY_ETH_SRC_MASK] = { .type = NL_A_UNSPEC,
> +  .min_len = ETH_ALEN,
> +

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

[Roi Dayan]

From: Paul Blakey 

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 
Signed-off-by: Shahar Klein 
Signed-off-by: Paul Blakey 
Reviewed-by: Roi Dayan 
Reviewed-by: Simon Horman 
---
 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 \
lib/if-notifier.c \
lib/if-notifier.h \
lib/netdev-linux.c \
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 79e8273..a6bb515 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -29,8 +29,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 #include 
 #include 
@@ -74,6 +72,7 @@
 #include "unaligned.h"
 #include "openvswitch/vlog.h"
 #include "util.h"
+#include "tc.h"
 
 VLOG_DEFINE_THIS_MODULE(netdev_linux);
 
@@ -434,18 +433,14 @@ static const struct tc_ops *const tcs[] = {
 NULL
 };
 
-static unsigned int tc_make_handle(unsigned int major, unsigned int minor);
-static unsigned int tc_get_major(unsigned int handle);
-static unsigned int tc_get_minor(unsigned int handle);
-
 static unsigned int tc_ticks_to_bytes(unsigned int rate, unsigned int ticks);
 static unsigned int tc_bytes_to_ticks(unsigned int rate, unsigned int size);
 static unsigned int tc_buffer_per_jiffy(unsigned int rate);
+static struct tcmsg *netdev_linux_tc_make_request(const struct netdev *,
+  int type,
+  unsigned int flags,
+  struct ofpbuf *);
 
-static struct tcmsg *tc_make_request(const struct netdev *, int type,
- unsigned int flags, struct ofpbuf *);
-static int tc_transact(struct ofpbuf *request, struct ofpbuf **replyp);
-static int tc_add_del_ingress_qdisc(struct netdev *netdev, bool add);
 static int tc_add_policer(struct netdev *,
   uint32_t kbits_rate, uint32_t kbits_burst);
 
@@ -2076,12 +2071,18 @@ netdev_linux_set_policing(struct netdev *netdev_,
 struct netdev_linux *netdev = netdev_linux_cast(netdev_);
 const char *netdev_name = netdev_get_name(netdev_);
 int error;
+int ifindex;
 
 kbits_burst = (!kbits_rate ? 0   /* Force to 0 if no rate specified. */
: !kbits_burst ? 8000 /* Default to 8000 kbits if 0. */
: kbits_burst);   /* Stick with user-specified value. */
 
 ovs_mutex_lock(>mutex);
+error = get_ifindex(netdev_, );
+if (error) {
+goto out;
+}
+
 if (netdev->cache_valid & VALID_POLICING) {
 error = netdev->netdev_policing_error;
 if (error || (netdev->kbits_rate == kbits_rate &&
@@ -2094,7 +2095,7 @@ netdev_linux_set_policing(struct netdev *netdev_,
 
 COVERAGE_INC(netdev_set_policing);
 /* Remove any existing ingress qdisc. */
-error = tc_add_del_ingress_qdisc(netdev_, false);
+error = tc_add_del_ingress_qdisc(ifindex, false);
 if (error) {
 VLOG_WARN_RL(, "%s: removing policing failed: %s",
  netdev_name, ovs_strerror(error));
@@ -2102,7 +2103,7 @@ netdev_linux_set_policing(struct netdev *netdev_,
 }
 
 if (kbits_rate) {
-error = tc_add_del_ingress_qdisc(netdev_, true);
+error = tc_add_del_ingress_qdisc(ifindex, true);
 if (error) {
 VLOG_WARN_RL(, "%s: adding policing qdisc failed: %s",
  netdev_name, ovs_strerror(error));
@@ -2371,7 +2372,7 @@ start_queue_dump(const struct netdev *netdev, struct 
queue_dump_state *state)
 struct ofpbuf request;
 struct tcmsg *tcmsg;
 
-tcmsg = tc_make_request(netdev, RTM_GETTCLASS, 0, );
+tcmsg = netdev_linux_tc_make_request(netdev, RTM_GETTCLASS, 0, );
 if (!tcmsg) {
 return false;
 }
@@ -2930,8 +2931,8 @@ codel_setup_qdisc__(struct netdev *netdev, uint32_t 
target, uint32_t limit,
 
 tc_del_qdisc(netdev);
 
-tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
-NLM_F_EXCL | NLM_F_CREATE, );
+tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
+ NLM_F_EXCL | NLM_F_CREATE, );
 if