On Wed, May 03, 2017 at 06:07:52PM +0300, Roi Dayan wrote:
> From: Paul Blakey <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Shahar Klein <[email protected]>
> Signed-off-by: Paul Blakey <[email protected]>
> Reviewed-by: Roi Dayan <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> ---
> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
