[PATCH v3 iproute2 0/3] Add support for ETF qdisc

[PATCH v3 iproute2 0/3] Add support for ETF qdisc

[Jesus Sanchez-Palencia]

Changes since v2:
 - Added man page for tc-etf.

The ETF (earliest txtime first) qdisc was recently merged into net-next
[1], so this patchset adds support for it through the tc command line
tool.

An initial man page is also provided.

The first commit in this series is adding an updated version of
include/uapi/linux/pkt_sched.h and is not meant to be merged. It's
provided here just as a convenience for those who want to easily build
this patchset.

[1] https://patchwork.ozlabs.org/cover/938991/

Jesus Sanchez-Palencia (2):
  uapi pkt_sched: Add etf info - DO NOT COMMIT
  man: Add initial manpage for tc-etf(8)

Vinicius Costa Gomes (1):
  tc: Add support for the ETF Qdisc

 include/uapi/linux/pkt_sched.h |  21 +++++
 man/man8/tc-etf.8              | 141 +++++++++++++++++++++++++++
 tc/Makefile                    |   1 +
 tc/q_etf.c                     | 168 +++++++++++++++++++++++++++++++++
 4 files changed, 331 insertions(+)
 create mode 100644 man/man8/tc-etf.8
 create mode 100644 tc/q_etf.c

-- 
2.18.0

[PATCH v3 iproute2 1/3] uapi pkt_sched: Add etf info - DO NOT COMMIT

[Jesus Sanchez-Palencia]

This should come from the next uapi headers update.
Sending it now just as a convenience so anyone can build tc with etf
and taprio support.

Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
 include/uapi/linux/pkt_sched.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 37b5096a..94911846 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -539,6 +539,7 @@ enum {
 	TCA_NETEM_LATENCY64,
 	TCA_NETEM_JITTER64,
 	TCA_NETEM_SLOT,
+	TCA_NETEM_SLOT_DIST,
 	__TCA_NETEM_MAX,
 };
 
@@ -581,6 +582,8 @@ struct tc_netem_slot {
 	__s64   max_delay;
 	__s32   max_packets;
 	__s32   max_bytes;
+	__s64	dist_delay; /* nsec */
+	__s64	dist_jitter; /* nsec */
 };
 
 enum {
@@ -934,4 +937,22 @@ enum {
 
 #define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
 
+
+/* ETF */
+struct tc_etf_qopt {
+	__s32 delta;
+	__s32 clockid;
+	__u32 flags;
+#define TC_ETF_DEADLINE_MODE_ON	BIT(0)
+#define TC_ETF_OFFLOAD_ON	BIT(1)
+};
+
+enum {
+	TCA_ETF_UNSPEC,
+	TCA_ETF_PARMS,
+	__TCA_ETF_MAX,
+};
+
+#define TCA_ETF_MAX (__TCA_ETF_MAX - 1)
+
 #endif
-- 
2.18.0

[PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[Jesus Sanchez-Palencia]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>

The "Earliest TxTime First" (ETF) queueing discipline allows precise
control of the transmission time of packets by providing a sorted
time-based scheduling of packets.

The syntax is:

tc qdisc add dev DEV parent NODE etf delta <DELTA>
                     clockid <CLOCKID> [offload] [deadline_mode]

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
 tc/Makefile |   1 +
 tc/q_etf.c  | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+)
 create mode 100644 tc/q_etf.c

diff --git a/tc/Makefile b/tc/Makefile
index dfd00267..4525c0fb 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -71,6 +71,7 @@ TCMODULES += q_clsact.o
 TCMODULES += e_bpf.o
 TCMODULES += f_matchall.o
 TCMODULES += q_cbs.o
+TCMODULES += q_etf.o
 
 TCSO :=
 ifeq ($(TC_CONFIG_ATM),y)
diff --git a/tc/q_etf.c b/tc/q_etf.c
new file mode 100644
index 00000000..5db1dd6f
--- /dev/null
+++ b/tc/q_etf.c
@@ -0,0 +1,168 @@
+/*
+ * q_etf.c		Earliest TxTime First (ETF).
+ *
+ *		This program is free software; you can redistribute it and/or
+ *		modify it under the terms of the GNU General Public License
+ *		as published by the Free Software Foundation; either version
+ *		2 of the License, or (at your option) any later version.
+ *
+ * Authors:	Vinicius Costa Gomes <vinicius.gomes@intel.com>
+ *		Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <arpa/inet.h>
+#include <string.h>
+
+#include "utils.h"
+#include "tc_util.h"
+
+#define CLOCKID_INVALID (-1)
+static void explain(void)
+{
+	fprintf(stderr, "Usage: ... etf delta NANOS clockid CLOCKID [offload] [deadline_mode]\n");
+	fprintf(stderr, "CLOCKID must be a valid SYS-V id (i.e. CLOCK_TAI)\n");
+}
+
+static void explain1(const char *arg, const char *val)
+{
+	fprintf(stderr, "etf: illegal value for \"%s\": \"%s\"\n", arg, val);
+}
+
+static void explain_clockid(const char *val)
+{
+	fprintf(stderr, "etf: illegal value for \"clockid\": \"%s\".\n", val);
+	fprintf(stderr, "It must be a valid SYS-V id (i.e. CLOCK_TAI)");
+}
+
+static int get_clockid(__s32 *val, const char *arg)
+{
+	const struct static_clockid {
+		const char *name;
+		clockid_t clockid;
+	} clockids_sysv[] = {
+		{ "CLOCK_REALTIME", CLOCK_REALTIME },
+		{ "CLOCK_TAI", CLOCK_TAI },
+		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
+		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
+		{ NULL }
+	};
+
+	const struct static_clockid *c;
+
+	for (c = clockids_sysv; c->name; c++) {
+		if (strncasecmp(c->name, arg, 25) == 0) {
+			*val = c->clockid;
+
+			return 0;
+		}
+	}
+
+	return -1;
+}
+
+
+static int etf_parse_opt(struct qdisc_util *qu, int argc,
+			 char **argv, struct nlmsghdr *n, const char *dev)
+{
+	struct tc_etf_qopt opt = {
+		.clockid = CLOCKID_INVALID,
+	};
+	struct rtattr *tail;
+
+	while (argc > 0) {
+		if (matches(*argv, "offload") == 0) {
+			if (opt.flags & TC_ETF_OFFLOAD_ON) {
+				fprintf(stderr, "etf: duplicate \"offload\" specification\n");
+				return -1;
+			}
+
+			opt.flags |= TC_ETF_OFFLOAD_ON;
+		} else if (matches(*argv, "deadline_mode") == 0) {
+			if (opt.flags & TC_ETF_DEADLINE_MODE_ON) {
+				fprintf(stderr, "etf: duplicate \"deadline_mode\" specification\n");
+				return -1;
+			}
+
+			opt.flags |= TC_ETF_DEADLINE_MODE_ON;
+		} else if (matches(*argv, "delta") == 0) {
+			NEXT_ARG();
+			if (opt.delta) {
+				fprintf(stderr, "etf: duplicate \"delta\" specification\n");
+				return -1;
+			}
+			if (get_s32(&opt.delta, *argv, 0)) {
+				explain1("delta", *argv);
+				return -1;
+			}
+		} else if (matches(*argv, "clockid") == 0) {
+			NEXT_ARG();
+			if (opt.clockid != CLOCKID_INVALID) {
+				fprintf(stderr, "etf: duplicate \"clockid\" specification\n");
+				return -1;
+			}
+			if (get_clockid(&opt.clockid, *argv)) {
+				explain_clockid(*argv);
+				return -1;
+			}
+		} else if (strcmp(*argv, "help") == 0) {
+			explain();
+			return -1;
+		} else {
+			fprintf(stderr, "etf: unknown parameter \"%s\"\n", *argv);
+			explain();
+			return -1;
+		}
+		argc--; argv++;
+	}
+
+	tail = NLMSG_TAIL(n);
+	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
+	addattr_l(n, 2024, TCA_ETF_PARMS, &opt, sizeof(opt));
+	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
+	return 0;
+}
+
+static int etf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
+{
+	struct rtattr *tb[TCA_ETF_MAX+1];
+	struct tc_etf_qopt *qopt;
+
+	if (opt == NULL)
+		return 0;
+
+	parse_rtattr_nested(tb, TCA_ETF_MAX, opt);
+
+	if (tb[TCA_ETF_PARMS] == NULL)
+		return -1;
+
+	qopt = RTA_DATA(tb[TCA_ETF_PARMS]);
+	if (RTA_PAYLOAD(tb[TCA_ETF_PARMS])  < sizeof(*qopt))
+		return -1;
+
+	if (qopt->clockid == CLOCKID_INVALID)
+		print_string(PRINT_ANY, "clockid", "clockid %s ", "invalid");
+	else
+		print_uint(PRINT_ANY, "clockid", "clockid %d ", qopt->clockid);
+
+	print_uint(PRINT_ANY, "delta", "delta %d ", qopt->delta);
+	print_string(PRINT_ANY, "offload", "offload %s ",
+				(qopt->flags & TC_ETF_OFFLOAD_ON) ? "on" : "off");
+	print_string(PRINT_ANY, "deadline_mode", "deadline_mode %s",
+				(qopt->flags & TC_ETF_DEADLINE_MODE_ON) ? "on" : "off");
+
+	return 0;
+}
+
+struct qdisc_util etf_qdisc_util = {
+	.id		= "etf",
+	.parse_qopt	= etf_parse_opt,
+	.print_qopt	= etf_print_opt,
+};
-- 
2.18.0

[PATCH v3 iproute2 3/3] man: Add initial manpage for tc-etf(8)

[Jesus Sanchez-Palencia]

Add an initial manpage for tc-etf covering all config options, basic
concepts and operation modes.

Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
 man/man8/tc-etf.8 | 141 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 141 insertions(+)
 create mode 100644 man/man8/tc-etf.8

diff --git a/man/man8/tc-etf.8 b/man/man8/tc-etf.8
new file mode 100644
index 00000000..30a12de7
--- /dev/null
+++ b/man/man8/tc-etf.8
@@ -0,0 +1,141 @@
+.TH ETF 8 "05 Jul 2018" "iproute2" "Linux"
+.SH NAME
+ETF \- Earliest TxTime First (ETF) Qdisc
+.SH SYNOPSIS
+.B tc qdisc ... dev
+dev
+.B parent
+classid
+.B [ handle
+major:
+.B ] etf clockid
+clockid
+.B [ delta
+delta_nsecs
+.B ] [ deadline_mode ]
+.B [ offload ]
+
+.SH DESCRIPTION
+The ETF (Earliest TxTime First) qdisc allows applications to control
+the instant when a packet should be dequeued from the traffic control
+layer into the netdevice. If
+.B offload
+is configured and supported by the network interface card, the it will
+also control when packets leave the network controller.
+
+ETF achieves that by buffering packets until a configurable time
+before their transmission time (i.e. txtime, or deadline), which can
+be configured through the
+.B delta
+option.
+
+The qdisc uses a rb-tree internally so packets are always 'ordered' by
+their txtime and will be dequeued following the (next) earliest txtime
+first.
+
+It relies on the SO_TXTIME socket option and the SCM_TXTIME CMSG in
+each packet field to configure the behavior of time dependent sockets:
+the clockid to be used as a reference, if the expected mode of txtime
+for that socket is deadline or strict mode, and if packet drops should
+be reported on the socket's error queue. See
+.BR socket(7)
+for more information.
+
+The etf qdisc will drop any packets with a txtime in the past, or if a
+packet expires while waiting for being dequeued.
+
+This queueing discipline is intended to be used by TSN (Time Sensitive
+Networking) applications, and it exposes a traffic shaping functionality
+that is commonly documented as "Launch Time" or "Time-Based Scheduling"
+by vendors and the documentation of network interface controllers.
+
+ETF is meant to be installed under another qdisc that maps packet flows
+to traffic classes, one example is
+.BR mqprio(8).
+
+.SH PARAMETERS
+.TP
+clockid
+.br
+Specifies the clock to be used by qdisc's internal timer for measuring
+time and scheduling events. The qdisc expects that packets passing
+through it to be using this same
+.B clockid
+as the reference of their txtime timestamps. It will drop packets
+coming from sockets that do not comply with that.
+
+For more information about time and clocks on Linux, please refer
+to
+.BR time(7)
+and
+.BR clock_gettime(3).
+
+.TP
+delta
+.br
+After enqueueing or dequeueing a packet, the qdisc will schedule its
+next wake-up time for the next txtime minus this delta value.
+This means
+.B delta
+can be used as a fudge factor for the scheduler latency of a system.
+This value must be specified in nanoseconds.
+The default value is 0 nanoseconds.
+
+.TP
+deadline_mode
+.br
+When
+.B deadline_mode
+is set, the qdisc will handle txtime with a different semantics,
+changed from a 'strict' transmission time to a deadline.
+In practice, this means during the dequeue flow
+.BR etf(8)
+will set the txtime of the packet being dequeued to 'now'.
+The default is for this option to be disabled.
+
+.TP
+offload
+.br
+When
+.B offload
+is set,
+.BR etf(8)
+will try to configure the network interface so time-based transmission
+arbitration is enabled in the controller. This feature is commonly
+referred to as "Launch Time" or "Time-Based Scheduling" by the
+documentation of network interface controllers.
+The default is for this option to be disabled.
+
+.SH EXAMPLES
+
+ETF is used to enforce a Quality of Service. It controls when each
+packets should be dequeued and transmitted, and can be used for
+limiting the data rate of a traffic class. To separate packets into
+traffic classes the user may choose
+.BR mqprio(8),
+and configure it like this:
+
+.EX
+# tc qdisc add dev eth0 handle 100: parent root mqprio num_tc 3 \\
+	map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \\
+	queues 1@0 1@1 2@2 \\
+	hw 0
+.EE
+.P
+To replace the current queueing discipline by ETF in traffic class
+number 0, issue:
+.P
+.EX
+# tc qdisc replace dev eth0 parent 100:1 etf \\
+	clockid CLOCK_TAI delta 300000 offload
+.EE
+
+With the options above, etf will be configured to use CLOCK_TAI as
+its clockid_t, will schedule packets for 300 us before their txtime,
+and will enable the functionality on that in the network interface
+card. Deadline mode will not be configured for this mode.
+
+.SH AUTHORS
+Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
+.br
+Vinicius Costa Gomes <vinicius.gomes@intel.com>
-- 
2.18.0

Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[David Ahern]

On 7/5/18 4:42 PM, Jesus Sanchez-Palencia wrote:

> +static int get_clockid(__s32 *val, const char *arg)
> +{
> +	const struct static_clockid {
> +		const char *name;
> +		clockid_t clockid;
> +	} clockids_sysv[] = {
> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
> +		{ "CLOCK_TAI", CLOCK_TAI },
> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
> +		{ NULL }
> +	};
> +
> +	const struct static_clockid *c;
> +
> +	for (c = clockids_sysv; c->name; c++) {
> +		if (strncasecmp(c->name, arg, 25) == 0) {

Why 25?

be nice to allow shortcuts -- e.g., just REALTIME or realtime.

> +			*val = c->clockid;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
> +
> +static int etf_parse_opt(struct qdisc_util *qu, int argc,
> +			 char **argv, struct nlmsghdr *n, const char *dev)
> +{
> +	struct tc_etf_qopt opt = {
> +		.clockid = CLOCKID_INVALID,
> +	};
> +	struct rtattr *tail;
> +
> +	while (argc > 0) {
> +		if (matches(*argv, "offload") == 0) {
> +			if (opt.flags & TC_ETF_OFFLOAD_ON) {
> +				fprintf(stderr, "etf: duplicate \"offload\" specification\n");
> +				return -1;
> +			}
> +
> +			opt.flags |= TC_ETF_OFFLOAD_ON;
> +		} else if (matches(*argv, "deadline_mode") == 0) {
> +			if (opt.flags & TC_ETF_DEADLINE_MODE_ON) {
> +				fprintf(stderr, "etf: duplicate \"deadline_mode\" specification\n");
> +				return -1;
> +			}
> +
> +			opt.flags |= TC_ETF_DEADLINE_MODE_ON;
> +		} else if (matches(*argv, "delta") == 0) {
> +			NEXT_ARG();
> +			if (opt.delta) {
> +				fprintf(stderr, "etf: duplicate \"delta\" specification\n");
> +				return -1;
> +			}
> +			if (get_s32(&opt.delta, *argv, 0)) {
> +				explain1("delta", *argv);
> +				return -1;
> +			}
> +		} else if (matches(*argv, "clockid") == 0) {
> +			NEXT_ARG();
> +			if (opt.clockid != CLOCKID_INVALID) {
> +				fprintf(stderr, "etf: duplicate \"clockid\" specification\n");
> +				return -1;
> +			}
> +			if (get_clockid(&opt.clockid, *argv)) {
> +				explain_clockid(*argv);
> +				return -1;
> +			}
> +		} else if (strcmp(*argv, "help") == 0) {
> +			explain();
> +			return -1;
> +		} else {
> +			fprintf(stderr, "etf: unknown parameter \"%s\"\n", *argv);
> +			explain();
> +			return -1;
> +		}
> +		argc--; argv++;
> +	}
> +
> +	tail = NLMSG_TAIL(n);
> +	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
> +	addattr_l(n, 2024, TCA_ETF_PARMS, &opt, sizeof(opt));
> +	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
> +	return 0;
> +}
> +
> +static int etf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
> +{
> +	struct rtattr *tb[TCA_ETF_MAX+1];
> +	struct tc_etf_qopt *qopt;
> +
> +	if (opt == NULL)
> +		return 0;
> +
> +	parse_rtattr_nested(tb, TCA_ETF_MAX, opt);
> +
> +	if (tb[TCA_ETF_PARMS] == NULL)
> +		return -1;
> +
> +	qopt = RTA_DATA(tb[TCA_ETF_PARMS]);
> +	if (RTA_PAYLOAD(tb[TCA_ETF_PARMS])  < sizeof(*qopt))
> +		return -1;
> +
> +	if (qopt->clockid == CLOCKID_INVALID)
> +		print_string(PRINT_ANY, "clockid", "clockid %s ", "invalid");
> +	else
> +		print_uint(PRINT_ANY, "clockid", "clockid %d ", qopt->clockid);

If you allow the user to input a string, then you should return it here too.

Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[Stephen Hemminger]

> diff --git a/tc/q_etf.c b/tc/q_etf.c
> new file mode 100644
> index 00000000..5db1dd6f
> --- /dev/null
> +++ b/tc/q_etf.c
> @@ -0,0 +1,168 @@
> +/*
> + * q_etf.c		Earliest TxTime First (ETF).
> + *
> + *		This program is free software; you can redistribute it and/or
> + *		modify it under the terms of the GNU General Public License
> + *		as published by the Free Software Foundation; either version
> + *		2 of the License, or (at your option) any later version.
> + *
> + * Authors:	Vinicius Costa Gomes <vinicius.gomes@intel.com>
> + *		Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
> + *
> + */


Please use SPDX tag rather than GPL boilerplate when adding new code.

> +static int get_clockid(__s32 *val, const char *arg)
> +{
> +	const struct static_clockid {
> +		const char *name;
> +		clockid_t clockid;
> +	} clockids_sysv[] = {
> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
> +		{ "CLOCK_TAI", CLOCK_TAI },
> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
> +		{ NULL }
> +	};
> +
> +	const struct static_clockid *c;
> +
> +	for (c = clockids_sysv; c->name; c++) {
> +		if (strncasecmp(c->name, arg, 25) == 0) {
> +			*val = c->clockid;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}

Internally, kernel must use ktime. For the userspace part the TC standard
is to use USER HZ of 100.

Please change user kernel API of this module to match other existing modules.
Doing something unique for this module is not necessary.

Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[Jesus Sanchez-Palencia]

Hi Stephen,


On 07/06/2018 02:32 PM, Stephen Hemminger wrote:
> 
>> diff --git a/tc/q_etf.c b/tc/q_etf.c
>> new file mode 100644
>> index 00000000..5db1dd6f
>> --- /dev/null
>> +++ b/tc/q_etf.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * q_etf.c		Earliest TxTime First (ETF).
>> + *
>> + *		This program is free software; you can redistribute it and/or
>> + *		modify it under the terms of the GNU General Public License
>> + *		as published by the Free Software Foundation; either version
>> + *		2 of the License, or (at your option) any later version.
>> + *
>> + * Authors:	Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> + *		Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
>> + *
>> + */
> 
> 
> Please use SPDX tag rather than GPL boilerplate when adding new code.

Sure, will do for v4.

> 
>> +static int get_clockid(__s32 *val, const char *arg)
>> +{
>> +	const struct static_clockid {
>> +		const char *name;
>> +		clockid_t clockid;
>> +	} clockids_sysv[] = {
>> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
>> +		{ "CLOCK_TAI", CLOCK_TAI },
>> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
>> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
>> +		{ NULL }
>> +	};
>> +
>> +	const struct static_clockid *c;
>> +
>> +	for (c = clockids_sysv; c->name; c++) {
>> +		if (strncasecmp(c->name, arg, 25) == 0) {
>> +			*val = c->clockid;
>> +
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -1;
>> +}
> 
> Internally, kernel must use ktime. For the userspace part the TC standard
> is to use USER HZ of 100.
> 
> Please change user kernel API of this module to match other existing modules.
> Doing something unique for this module is not necessary.


I don't follow you on the above, sorry.

The qdisc must be configured with a valid clockid_t. This type is used by both
userspace and kernel.

As requested before, we made the tc etf command line interface more
user-friendly and allowed for users to input the clock name as a string. The
code above is just lookup table converting the string into a a valid clockid_t
so we can then pass it to the kernel.

There is no ktime or any other timestamp type above, only clockid_t.

Can you please clarify what your request is?


Thanks,
Jesus

Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[Jesus Sanchez-Palencia]

Hi David,


On 07/06/2018 08:58 AM, David Ahern wrote:
> On 7/5/18 4:42 PM, Jesus Sanchez-Palencia wrote:
> 
>> +static int get_clockid(__s32 *val, const char *arg)
>> +{
>> +	const struct static_clockid {
>> +		const char *name;
>> +		clockid_t clockid;
>> +	} clockids_sysv[] = {
>> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
>> +		{ "CLOCK_TAI", CLOCK_TAI },
>> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
>> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
>> +		{ NULL }
>> +	};
>> +
>> +	const struct static_clockid *c;
>> +
>> +	for (c = clockids_sysv; c->name; c++) {
>> +		if (strncasecmp(c->name, arg, 25) == 0) {
> 
> Why 25?


That was just an upper bound giving some room beyond the longest
clockid name we have today. Should I add a define MAX_CLOCK_NAME ?


> 
> be nice to allow shortcuts -- e.g., just REALTIME or realtime.


I'd rather just keep it as is and use the names as they are defined for
everything else (i.e. CLOCK_REALTIME), unless there are some strong objections.



> 
>> +			*val = c->clockid;
>> +
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> +
>> +static int etf_parse_opt(struct qdisc_util *qu, int argc,
>> +			 char **argv, struct nlmsghdr *n, const char *dev)
>> +{
>> +	struct tc_etf_qopt opt = {
>> +		.clockid = CLOCKID_INVALID,
>> +	};
>> +	struct rtattr *tail;
>> +
>> +	while (argc > 0) {
>> +		if (matches(*argv, "offload") == 0) {
>> +			if (opt.flags & TC_ETF_OFFLOAD_ON) {
>> +				fprintf(stderr, "etf: duplicate \"offload\" specification\n");
>> +				return -1;
>> +			}
>> +
>> +			opt.flags |= TC_ETF_OFFLOAD_ON;
>> +		} else if (matches(*argv, "deadline_mode") == 0) {
>> +			if (opt.flags & TC_ETF_DEADLINE_MODE_ON) {
>> +				fprintf(stderr, "etf: duplicate \"deadline_mode\" specification\n");
>> +				return -1;
>> +			}
>> +
>> +			opt.flags |= TC_ETF_DEADLINE_MODE_ON;
>> +		} else if (matches(*argv, "delta") == 0) {
>> +			NEXT_ARG();
>> +			if (opt.delta) {
>> +				fprintf(stderr, "etf: duplicate \"delta\" specification\n");
>> +				return -1;
>> +			}
>> +			if (get_s32(&opt.delta, *argv, 0)) {
>> +				explain1("delta", *argv);
>> +				return -1;
>> +			}
>> +		} else if (matches(*argv, "clockid") == 0) {
>> +			NEXT_ARG();
>> +			if (opt.clockid != CLOCKID_INVALID) {
>> +				fprintf(stderr, "etf: duplicate \"clockid\" specification\n");
>> +				return -1;
>> +			}
>> +			if (get_clockid(&opt.clockid, *argv)) {
>> +				explain_clockid(*argv);
>> +				return -1;
>> +			}
>> +		} else if (strcmp(*argv, "help") == 0) {
>> +			explain();
>> +			return -1;
>> +		} else {
>> +			fprintf(stderr, "etf: unknown parameter \"%s\"\n", *argv);
>> +			explain();
>> +			return -1;
>> +		}
>> +		argc--; argv++;
>> +	}
>> +
>> +	tail = NLMSG_TAIL(n);
>> +	addattr_l(n, 1024, TCA_OPTIONS, NULL, 0);
>> +	addattr_l(n, 2024, TCA_ETF_PARMS, &opt, sizeof(opt));
>> +	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
>> +	return 0;
>> +}
>> +
>> +static int etf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>> +{
>> +	struct rtattr *tb[TCA_ETF_MAX+1];
>> +	struct tc_etf_qopt *qopt;
>> +
>> +	if (opt == NULL)
>> +		return 0;
>> +
>> +	parse_rtattr_nested(tb, TCA_ETF_MAX, opt);
>> +
>> +	if (tb[TCA_ETF_PARMS] == NULL)
>> +		return -1;
>> +
>> +	qopt = RTA_DATA(tb[TCA_ETF_PARMS]);
>> +	if (RTA_PAYLOAD(tb[TCA_ETF_PARMS])  < sizeof(*qopt))
>> +		return -1;
>> +
>> +	if (qopt->clockid == CLOCKID_INVALID)
>> +		print_string(PRINT_ANY, "clockid", "clockid %s ", "invalid");
>> +	else
>> +		print_uint(PRINT_ANY, "clockid", "clockid %d ", qopt->clockid);
> 
> If you allow the user to input a string, then you should return it here too.


Ok, will fix.

Thanks,
Jesus

Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[David Ahern]

On 7/9/18 9:48 AM, Jesus Sanchez-Palencia wrote:
> Hi David,
> 
> 
> On 07/06/2018 08:58 AM, David Ahern wrote:
>> On 7/5/18 4:42 PM, Jesus Sanchez-Palencia wrote:
>>
>>> +static int get_clockid(__s32 *val, const char *arg)
>>> +{
>>> +	const struct static_clockid {
>>> +		const char *name;
>>> +		clockid_t clockid;
>>> +	} clockids_sysv[] = {
>>> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
>>> +		{ "CLOCK_TAI", CLOCK_TAI },
>>> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
>>> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
>>> +		{ NULL }
>>> +	};
>>> +
>>> +	const struct static_clockid *c;
>>> +
>>> +	for (c = clockids_sysv; c->name; c++) {
>>> +		if (strncasecmp(c->name, arg, 25) == 0) {
>>
>> Why 25?
> 
> 
> That was just an upper bound giving some room beyond the longest
> clockid name we have today. Should I add a define MAX_CLOCK_NAME ?

why not just strcasecmp? using the 'n' variant with n > strlen of either
argument seems pointless.

> 
> 
>>
>> be nice to allow shortcuts -- e.g., just REALTIME or realtime.
> 
> 
> I'd rather just keep it as is and use the names as they are defined for
> everything else (i.e. CLOCK_REALTIME), unless there are some strong objections.

An all caps argument is unnecessary work on the pinky finger and the
CLOCK_ prefix is redundant to the keyword. Really, just a thought on
making it easier for users. A CLI argument does not need to maintain a
1:1 with code names.

Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc

[Jesus Sanchez-Palencia]

On 07/09/2018 10:32 AM, David Ahern wrote:
> On 7/9/18 9:48 AM, Jesus Sanchez-Palencia wrote:
>> Hi David,
>>
>>
>> On 07/06/2018 08:58 AM, David Ahern wrote:
>>> On 7/5/18 4:42 PM, Jesus Sanchez-Palencia wrote:
>>>
>>>> +static int get_clockid(__s32 *val, const char *arg)
>>>> +{
>>>> +	const struct static_clockid {
>>>> +		const char *name;
>>>> +		clockid_t clockid;
>>>> +	} clockids_sysv[] = {
>>>> +		{ "CLOCK_REALTIME", CLOCK_REALTIME },
>>>> +		{ "CLOCK_TAI", CLOCK_TAI },
>>>> +		{ "CLOCK_BOOTTIME", CLOCK_BOOTTIME },
>>>> +		{ "CLOCK_MONOTONIC", CLOCK_MONOTONIC },
>>>> +		{ NULL }
>>>> +	};
>>>> +
>>>> +	const struct static_clockid *c;
>>>> +
>>>> +	for (c = clockids_sysv; c->name; c++) {
>>>> +		if (strncasecmp(c->name, arg, 25) == 0) {
>>>
>>> Why 25?
>>
>>
>> That was just an upper bound giving some room beyond the longest
>> clockid name we have today. Should I add a define MAX_CLOCK_NAME ?
> 
> why not just strcasecmp? using the 'n' variant with n > strlen of either
> argument seems pointless.


Ok, will fix.


> 
>>
>>
>>>
>>> be nice to allow shortcuts -- e.g., just REALTIME or realtime.
>>
>>
>> I'd rather just keep it as is and use the names as they are defined for
>> everything else (i.e. CLOCK_REALTIME), unless there are some strong objections.
> 
> An all caps argument is unnecessary work on the pinky finger and the
> CLOCK_ prefix is redundant to the keyword. Really, just a thought on
> making it easier for users. A CLI argument does not need to maintain a
> 1:1 with code names.


Lower case already works given the strncasecmp() usage but, fair enough, I will
modify the implementation so it accepts both CLOCK_FOO or FOO (lower case
included), and will make it print one of the two strings during print_opt().


Thanks,
Jesus