Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc
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
Re: [PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc
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
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(, *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(, *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, , 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
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 ..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
>> + * Jesus Sanchez-Palencia
>> + *
>> + */
>
>
> 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
> diff --git a/tc/q_etf.c b/tc/q_etf.c
> new file mode 100644
> index ..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
> + * Jesus Sanchez-Palencia
> + *
> + */
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
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(, *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(, *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, , 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.
[PATCH v3 iproute2 2/3] tc: Add support for the ETF Qdisc
From: Vinicius Costa Gomes
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
clockid [offload] [deadline_mode]
Signed-off-by: Vinicius Costa Gomes
Signed-off-by: Jesus Sanchez-Palencia
---
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 ..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
+ * Jesus Sanchez-Palencia
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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(, *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(, *argv)) {
+ explain_clockid(*argv);
+ return -1;
+ }
+ } else if (strcmp(*argv, "help") == 0) {
+ explain();
+ return -1;
+
