Re: [ovs-dev] [PATCH ovs V8 01/26] tc: Add tc flower interface
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
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, &request); +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC, + NLM_F_EXCL | NLM_F_CREATE, &request); 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, &request); - +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC, + NLM_F_EXCL | NLM_F_CREATE, &request); 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, &request); - +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTCLASS, + NLM_F_CREATE, &request); 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 = E
Re: [ovs-dev] [PATCH ovs V8 01/26] tc: Add tc flower interface
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, &request); > +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC, > + NLM_F_EXCL | NLM_F_CREATE, > &request); 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, &request); > - > +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC, > + NLM_F_EXCL | NLM_F_CREATE, > &request); > 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, &request); > - > +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTCLASS, > + NLM_F_CREATE, &request); > 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, > + .optional = true, }, > +[TCA_FLOWER_KEY_ETH_DST_MASK]
[ovs-dev] [PATCH ovs V8 01/26] tc: Add tc flower interface
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(&netdev->mutex); +error = get_ifindex(netdev_, &ifindex); +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(&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(&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, &request); +tcmsg = netdev_linux_tc_make_request(netdev, RTM_GETTCLASS, 0, &request); 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, &request); +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC, + NLM_F_EXCL | NLM_F_CREATE, &request); if (!tcmsg) { return ENODEV; } @@ -3148,8 +3149,8 @@ fqcodel_setup_qdisc__