On Wed, May 03, 2017 at 06:07:52PM +0300, Roi Dayan wrote: > From: Paul Blakey <pa...@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 <shah...@mellanox.com> > Signed-off-by: Shahar Klein <shah...@mellanox.com> > Signed-off-by: Paul Blakey <pa...@mellanox.com> > Reviewed-by: Roi Dayan <r...@mellanox.com> > Reviewed-by: Simon Horman <simon.hor...@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. 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 0000000..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] = { .type = NL_A_UNSPEC, > + .min_len = ETH_ALEN, > + .optional = true, }, > + [TCA_FLOWER_KEY_ETH_TYPE] = { .type = NL_A_U16, .optional = false, }, > + [TCA_FLOWER_FLAGS] = { .type = NL_A_U32, .optional = false, }, > + [TCA_FLOWER_ACT] = { .type = NL_A_NESTED, .optional = false, }, > + [TCA_FLOWER_KEY_IP_PROTO] = { .type = NL_A_U8, .optional = true, }, > + [TCA_FLOWER_KEY_IPV4_SRC] = { .type = NL_A_U32, .optional = true, }, > + [TCA_FLOWER_KEY_IPV4_DST] = {.type = NL_A_U32, .optional = true, }, > + [TCA_FLOWER_KEY_IPV4_SRC_MASK] = { .type = NL_A_U32, .optional = true, }, > + [TCA_FLOWER_KEY_IPV4_DST_MASK] = { .type = NL_A_U32, .optional = true, }, > + [TCA_FLOWER_KEY_IPV6_SRC] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_IPV6_DST] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_IPV6_SRC_MASK] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_IPV6_DST_MASK] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_TCP_SRC] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_TCP_DST] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_TCP_SRC_MASK] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_TCP_DST_MASK] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_UDP_SRC] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_UDP_DST] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_UDP_SRC_MASK] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_UDP_DST_MASK] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_VLAN_ID] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_VLAN_PRIO] = { .type = NL_A_U8, .optional = true, }, > + [TCA_FLOWER_KEY_VLAN_ETH_TYPE] = { .type = NL_A_U16, .optional = true, }, > + [TCA_FLOWER_KEY_ENC_KEY_ID] = { .type = NL_A_BE32, .optional = true, }, > + [TCA_FLOWER_KEY_ENC_IPV4_SRC] = { .type = NL_A_BE32, .optional = true, }, > + [TCA_FLOWER_KEY_ENC_IPV4_DST] = { .type = NL_A_BE32, .optional = true, }, > + [TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK] = { .type = NL_A_BE32, .optional = > true, }, > + [TCA_FLOWER_KEY_ENC_IPV4_DST_MASK] = { .type = NL_A_BE32, .optional = > true, }, I am wondering why the type of the above IPV4 attributes are NL_A_BE32 while those further are are NL_A_U32. > + [TCA_FLOWER_KEY_ENC_IPV6_SRC] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_ENC_IPV6_DST] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_ENC_IPV6_SRC_MASK] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct > in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_ENC_IPV6_DST_MASK] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct > in6_addr), > + .optional = true, }, > + [TCA_FLOWER_KEY_ENC_UDP_DST_PORT] = { .type = NL_A_BE16, > + .optional = true, }, > +}; ... > +static void > +nl_parse_flower_ip(struct nlattr **attrs, struct tc_flower *flower) { ... > + if (ip_proto == IPPROTO_TCP) { > + if (attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]) { > + key->src_port = > + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC]); > + mask->src_port = > + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]); > + } > + if (attrs[TCA_FLOWER_KEY_TCP_DST_MASK]) { > + key->dst_port = > + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST]); > + mask->dst_port = > + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST_MASK]); > + } > + } else if (ip_proto == IPPROTO_UDP) { > + if (attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]) { > + key->src_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC]); > + mask->src_port = > + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]); > + } > + if (attrs[TCA_FLOWER_KEY_UDP_DST_MASK]) { > + key->dst_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST]); > + mask->dst_port = > + nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST_MASK]); > + } > + } As noted by Flavio Leitner in his review of v7 it seems likely that SCTP could trivially be supported. ... > +static int > +nl_parse_act_drop(struct nlattr *options, struct tc_flower *flower) > +{ > + struct nlattr *gact_attrs[ARRAY_SIZE(gact_policy)]; > + const struct tc_gact *p; > + struct nlattr *gact_parms; > + const struct tcf_t *tm; > + > + if (!nl_parse_nested(options, gact_policy, gact_attrs, > + ARRAY_SIZE(gact_policy))) { > + VLOG_ERR_RL(&parse_err, "failed to parse gact action options"); > + return EPROTO; > + } > + > + gact_parms = gact_attrs[TCA_GACT_PARMS]; > + p = nl_attr_get_unspec(gact_parms, sizeof *p); > + > + if (p->action == TC_ACT_SHOT) { > + } else { The following seems more logical to me: if (p->action != TC_ACT_SHOT) { > + VLOG_ERR_RL(&parse_err, "unknown gact action: %d", p->action); > + return EINVAL; > + } > + > + tm = nl_attr_get_unspec(gact_attrs[TCA_GACT_TM], sizeof *tm); > + nl_parse_tcf(tm, flower); > + > + return 0; > +} ... _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev