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 <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.
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 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.
no reason. probably a mistake. should be U32 as in the kernel.
+ [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.
ok. probably missed that email. I'll check SCTP.
...+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) {
right. missed that.
+ 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
