[PATCH 0/3] net/sched: netem cleanups

[PATCH 0/3] net/sched: netem cleanups

[Stephen Hemminger]

Netem needs some minor updates.

Stephen Hemminger (3):
  net/sched: netem: use extack
  net/sched: netem: get rid of unnecesary version message
  net/sched: netem: update intro comment

 net/sched/sch_netem.c | 49 ++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH 1/3] net/sched: netem: use extack

[Stephen Hemminger]

The error handling in netem predates introduction of extack,
and was mostly using pr_info(). Use extack to put errors in
result rather than console log.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fa678eb88528..7c37a69aba0e 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -782,15 +782,18 @@ static void dist_free(struct disttable *d)
  * signed 16 bit values.
  */
 
-static int get_dist_table(struct disttable **tbl, const struct nlattr *attr)
+static int get_dist_table(struct disttable **tbl, const struct nlattr *attr,
+			  struct netlink_ext_ack *extack)
 {
 	size_t n = nla_len(attr)/sizeof(__s16);
 	const __s16 *data = nla_data(attr);
 	struct disttable *d;
 	int i;
 
-	if (!n || n > NETEM_DIST_MAX)
+	if (!n || n > NETEM_DIST_MAX) {
+		NL_SET_ERR_MSG_FMT_MOD(extack, "invalid distribution table size: %zu", n);
 		return -EINVAL;
+	}
 
 	d = kvmalloc(struct_size(d, table, n), GFP_KERNEL);
 	if (!d)
@@ -865,7 +868,8 @@ static void get_rate(struct netem_sched_data *q, const struct nlattr *attr)
 		q->cell_size_reciprocal = (struct reciprocal_value) { 0 };
 }
 
-static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
+static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr,
+			struct netlink_ext_ack *extack)
 {
 	const struct nlattr *la;
 	int rem;
@@ -878,7 +882,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 			const struct tc_netem_gimodel *gi = nla_data(la);
 
 			if (nla_len(la) < sizeof(struct tc_netem_gimodel)) {
-				pr_info("netem: incorrect gi model size\n");
+				NL_SET_ERR_MSG_MOD(extack, "incorrect GI model size");
 				return -EINVAL;
 			}
 
@@ -897,7 +901,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 			const struct tc_netem_gemodel *ge = nla_data(la);
 
 			if (nla_len(la) < sizeof(struct tc_netem_gemodel)) {
-				pr_info("netem: incorrect ge model size\n");
+				NL_SET_ERR_MSG_MOD(extack, "incorrect GE model size");
 				return -EINVAL;
 			}
 
@@ -911,7 +915,7 @@ static int get_loss_clg(struct netem_sched_data *q, const struct nlattr *attr)
 		}
 
 		default:
-			pr_info("netem: unknown loss type %u\n", type);
+			NL_SET_ERR_MSG_FMT_MOD(extack, "unknown loss type: %u", type);
 			return -EINVAL;
 		}
 	}
@@ -934,12 +938,13 @@ static const struct nla_policy netem_policy[TCA_NETEM_MAX + 1] = {
 };
 
 static int parse_attr(struct nlattr *tb[], int maxtype, struct nlattr *nla,
-		      const struct nla_policy *policy, int len)
+		      const struct nla_policy *policy, int len,
+		      struct netlink_ext_ack *extack)
 {
 	int nested_len = nla_len(nla) - NLA_ALIGN(len);
 
 	if (nested_len < 0) {
-		pr_info("netem: invalid attributes len %d\n", nested_len);
+		NL_SET_ERR_MSG_MOD(extack, "invalid nested attribute length");
 		return -EINVAL;
 	}
 
@@ -966,18 +971,18 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	int ret;
 
 	qopt = nla_data(opt);
-	ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt));
+	ret = parse_attr(tb, TCA_NETEM_MAX, opt, netem_policy, sizeof(*qopt), extack);
 	if (ret < 0)
 		return ret;
 
 	if (tb[TCA_NETEM_DELAY_DIST]) {
-		ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST]);
+		ret = get_dist_table(&delay_dist, tb[TCA_NETEM_DELAY_DIST], extack);
 		if (ret)
 			goto table_free;
 	}
 
 	if (tb[TCA_NETEM_SLOT_DIST]) {
-		ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST]);
+		ret = get_dist_table(&slot_dist, tb[TCA_NETEM_SLOT_DIST], extack);
 		if (ret)
 			goto table_free;
 	}
@@ -988,7 +993,7 @@ static int netem_change(struct Qdisc *sch, struct nlattr *opt,
 	old_loss_model = q->loss_model;
 
 	if (tb[TCA_NETEM_LOSS]) {
-		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS]);
+		ret = get_loss_clg(q, tb[TCA_NETEM_LOSS], extack);
 		if (ret) {
 			q->loss_model = old_loss_model;
 			q->clg = old_clg;
@@ -1068,18 +1073,16 @@ static int netem_init(struct Qdisc *sch, struct nlattr *opt,
 		      struct netlink_ext_ack *extack)
 {
 	struct netem_sched_data *q = qdisc_priv(sch);
-	int ret;
 
 	qdisc_watchdog_init(&q->watchdog, sch);
 
-	if (!opt)
+	if (!opt) {
+		NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters");
 		return -EINVAL;
+	}
 
 	q->loss_model = CLG_RANDOM;
-	ret = netem_change(sch, opt, extack);
-	if (ret)
-		pr_info("netem: change failed\n");
-	return ret;
+	return netem_change(sch, opt, extack);
 }
 
 static void netem_destroy(struct Qdisc *sch)
-- 
2.43.0

[PATCH 2/3] net/sched: netem: get rid of unnecesary version message

[Stephen Hemminger]

The version of netem module is irrelevant and was never useful.
Remove it.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 7c37a69aba0e..f712d03ad854 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -26,8 +26,6 @@
 #include <net/pkt_sched.h>
 #include <net/inet_ecn.h>
 
-#define VERSION "1.3"
-
 /*	Network Emulation Queuing algorithm.
 	====================================
 
@@ -1300,13 +1298,14 @@ static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
 
 static int __init netem_module_init(void)
 {
-	pr_info("netem: version " VERSION "\n");
 	return register_qdisc(&netem_qdisc_ops);
 }
+
 static void __exit netem_module_exit(void)
 {
 	unregister_qdisc(&netem_qdisc_ops);
 }
+
 module_init(netem_module_init)
 module_exit(netem_module_exit)
 MODULE_LICENSE("GPL");
-- 
2.43.0

[PATCH 3/3] net/sched: netem: update intro comment

[Stephen Hemminger]

Netem originally used a nested qdisc to handle rate limiting,
but that has been replaced (for many years) with an internal FIFO.
Update the intro comment to reflect this.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 net/sched/sch_netem.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index f712d03ad854..cc3df77503b3 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -44,9 +44,8 @@
 	 duplication, and reordering can also be emulated.
 
 	 This qdisc does not do classification that can be handled in
-	 layering other disciplines.  It does not need to do bandwidth
-	 control either since that can be handled by using token
-	 bucket or other rate control.
+	 layering other disciplines. Netem includes an optional internal
+	 rate limiter (tfifo) based on next time to send.
 
      Correlated Loss Generator models
 
-- 
2.43.0

Re: [PATCH 1/3] net/sched: netem: use extack

[Jiri Pirko]

Thu, Feb 01, 2024 at 04:45:58AM CET, stephen@networkplumber.org wrote:
>The error handling in netem predates introduction of extack,
>and was mostly using pr_info(). Use extack to put errors in
>result rather than console log.
>

[...]

>@@ -1068,18 +1073,16 @@ static int netem_init(struct Qdisc *sch, struct nlattr *opt,
> 		      struct netlink_ext_ack *extack)
> {
> 	struct netem_sched_data *q = qdisc_priv(sch);
>-	int ret;
> 
> 	qdisc_watchdog_init(&q->watchdog, sch);
> 
>-	if (!opt)
>+	if (!opt) {
>+		NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters");

Drop "Netem " here.

Otherwise, this looks fine.

> 		return -EINVAL;
>+	}
> 
> 	q->loss_model = CLG_RANDOM;
>-	ret = netem_change(sch, opt, extack);
>-	if (ret)
>-		pr_info("netem: change failed\n");
>-	return ret;
>+	return netem_change(sch, opt, extack);
> }
> 
> static void netem_destroy(struct Qdisc *sch)
>-- 
>2.43.0
>
>

Re: [PATCH 2/3] net/sched: netem: get rid of unnecesary version message

[Jiri Pirko]

Thu, Feb 01, 2024 at 04:45:59AM CET, stephen@networkplumber.org wrote:
>The version of netem module is irrelevant and was never useful.
>Remove it.
>
>Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>---
> net/sched/sch_netem.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
>diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
>index 7c37a69aba0e..f712d03ad854 100644
>--- a/net/sched/sch_netem.c
>+++ b/net/sched/sch_netem.c
>@@ -26,8 +26,6 @@
> #include <net/pkt_sched.h>
> #include <net/inet_ecn.h>
> 
>-#define VERSION "1.3"
>-
> /*	Network Emulation Queuing algorithm.
> 	====================================
> 
>@@ -1300,13 +1298,14 @@ static struct Qdisc_ops netem_qdisc_ops __read_mostly = {
> 
> static int __init netem_module_init(void)
> {
>-	pr_info("netem: version " VERSION "\n");
> 	return register_qdisc(&netem_qdisc_ops);
> }
>+
> static void __exit netem_module_exit(void)
> {
> 	unregister_qdisc(&netem_qdisc_ops);
> }
>+

These whitespace changes look unrelated to the patch. Anyway:

Reviewed-by: Jiri Pirko <jiri@nvidia.com>


> module_init(netem_module_init)
> module_exit(netem_module_exit)
> MODULE_LICENSE("GPL");
>-- 
>2.43.0
>
>

Re: [PATCH 3/3] net/sched: netem: update intro comment

[Jiri Pirko]

Thu, Feb 01, 2024 at 04:46:00AM CET, stephen@networkplumber.org wrote:
>Netem originally used a nested qdisc to handle rate limiting,
>but that has been replaced (for many years) with an internal FIFO.
>Update the intro comment to reflect this.
>
>Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Re: [PATCH 1/3] net/sched: netem: use extack

[Jakub Kicinski]

On Thu, 1 Feb 2024 10:30:27 +0100 Jiri Pirko wrote:
> Thu, Feb 01, 2024 at 04:45:58AM CET, stephen@networkplumber.org wrote:
> >-	if (!opt)
> >+	if (!opt) {
> >+		NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters");  
> 
> Drop "Netem " here.
> 
> Otherwise, this looks fine.

Looks like most sch's require opt. Would it be a bad idea to pull 
the check out to the caller? Minor simplification, plus the caller
has the outer message so they can use NL_SET_ERR_ATTR_MISS() and
friends.


$ git grep -A1 'if (!opt)' -- net/sched/
net/sched/cls_fw.c:     if (!opt)
net/sched/cls_fw.c-             return handle ? -EINVAL : 0; /* Succeed if it is old method. */
--
net/sched/cls_u32.c:    if (!opt) {
net/sched/cls_u32.c-            if (handle) {
--
net/sched/sch_cbs.c:    if (!opt) {
net/sched/sch_cbs.c-            NL_SET_ERR_MSG(extack, "Missing CBS qdisc options  which are mandatory");
--
net/sched/sch_drr.c:    if (!opt) {
net/sched/sch_drr.c-            NL_SET_ERR_MSG(extack, "DRR options are required for this operation");
--
net/sched/sch_etf.c:    if (!opt) {
net/sched/sch_etf.c-            NL_SET_ERR_MSG(extack,
--
net/sched/sch_ets.c:    if (!opt) {
net/sched/sch_ets.c-            NL_SET_ERR_MSG(extack, "ETS options are required for this operation");
--
net/sched/sch_ets.c:    if (!opt)
net/sched/sch_ets.c-            return -EINVAL;
--
net/sched/sch_gred.c:   if (!opt)
net/sched/sch_gred.c-           return -EINVAL;
--
net/sched/sch_htb.c:    if (!opt)
net/sched/sch_htb.c-            return -EINVAL;
--
net/sched/sch_htb.c:    if (!opt)
net/sched/sch_htb.c-            goto failure;
--
net/sched/sch_multiq.c: if (!opt)
net/sched/sch_multiq.c-         return -EINVAL;
--
net/sched/sch_netem.c:  if (!opt)
net/sched/sch_netem.c-          return -EINVAL;
--
net/sched/sch_prio.c:   if (!opt)
net/sched/sch_prio.c-           return -EINVAL;
--
net/sched/sch_red.c:    if (!opt)
net/sched/sch_red.c-            return -EINVAL;
--
net/sched/sch_skbprio.c:        if (!opt)
net/sched/sch_skbprio.c-                return 0;
--
net/sched/sch_taprio.c: if (!opt)
net/sched/sch_taprio.c-         return -EINVAL;
--
net/sched/sch_tbf.c:    if (!opt)
net/sched/sch_tbf.c-            return -EINVAL;

Re: [PATCH 1/3] net/sched: netem: use extack

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> On Thu, 1 Feb 2024 10:30:27 +0100 Jiri Pirko wrote:
>> Thu, Feb 01, 2024 at 04:45:58AM CET, stephen@networkplumber.org wrote:
>> >-	if (!opt)
>> >+	if (!opt) {
>> >+		NL_SET_ERR_MSG_MOD(extack, "Netem missing required parameters");  
>> 
>> Drop "Netem " here.
>> 
>> Otherwise, this looks fine.
>
> Looks like most sch's require opt. Would it be a bad idea to pull 
> the check out to the caller? Minor simplification, plus the caller
> has the outer message so they can use NL_SET_ERR_ATTR_MISS() and
> friends.

There's also these which maybe complicates things:

$ git grep -A1 'if (opt == NULL)' -- net/sched/
net/sched/cls_flow.c:   if (opt == NULL)
net/sched/cls_flow.c-           return -EINVAL;
--
net/sched/sch_choke.c:  if (opt == NULL)
net/sched/sch_choke.c-          return -EINVAL;
--
net/sched/sch_fifo.c:   if (opt == NULL) {
net/sched/sch_fifo.c-           u32 limit = qdisc_dev(sch)->tx_queue_len;
--
net/sched/sch_hfsc.c:   if (opt == NULL)
net/sched/sch_hfsc.c-           return -EINVAL;
--
net/sched/sch_plug.c:   if (opt == NULL) {
net/sched/sch_plug.c-           q->limit = qdisc_dev(sch)->tx_queue_len

I'm in favour of qdisc specific extack messages.

> $ git grep -A1 'if (!opt)' -- net/sched/
> net/sched/cls_fw.c:     if (!opt)
> net/sched/cls_fw.c-             return handle ? -EINVAL : 0; /* Succeed if it is old method. */
> --
> net/sched/cls_u32.c:    if (!opt) {
> net/sched/cls_u32.c-            if (handle) {
> --
> net/sched/sch_cbs.c:    if (!opt) {
> net/sched/sch_cbs.c-            NL_SET_ERR_MSG(extack, "Missing CBS qdisc options  which are mandatory");
> --
> net/sched/sch_drr.c:    if (!opt) {
> net/sched/sch_drr.c-            NL_SET_ERR_MSG(extack, "DRR options are required for this operation");
> --
> net/sched/sch_etf.c:    if (!opt) {
> net/sched/sch_etf.c-            NL_SET_ERR_MSG(extack,
> --
> net/sched/sch_ets.c:    if (!opt) {
> net/sched/sch_ets.c-            NL_SET_ERR_MSG(extack, "ETS options are required for this operation");
> --
> net/sched/sch_ets.c:    if (!opt)
> net/sched/sch_ets.c-            return -EINVAL;
> --
> net/sched/sch_gred.c:   if (!opt)
> net/sched/sch_gred.c-           return -EINVAL;
> --
> net/sched/sch_htb.c:    if (!opt)
> net/sched/sch_htb.c-            return -EINVAL;
> --
> net/sched/sch_htb.c:    if (!opt)
> net/sched/sch_htb.c-            goto failure;
> --
> net/sched/sch_multiq.c: if (!opt)
> net/sched/sch_multiq.c-         return -EINVAL;
> --
> net/sched/sch_netem.c:  if (!opt)
> net/sched/sch_netem.c-          return -EINVAL;
> --
> net/sched/sch_prio.c:   if (!opt)
> net/sched/sch_prio.c-           return -EINVAL;
> --
> net/sched/sch_red.c:    if (!opt)
> net/sched/sch_red.c-            return -EINVAL;
> --
> net/sched/sch_skbprio.c:        if (!opt)
> net/sched/sch_skbprio.c-                return 0;
> --
> net/sched/sch_taprio.c: if (!opt)
> net/sched/sch_taprio.c-         return -EINVAL;
> --
> net/sched/sch_tbf.c:    if (!opt)
> net/sched/sch_tbf.c-            return -EINVAL;

Re: [PATCH 1/3] net/sched: netem: use extack

[Jakub Kicinski]

On Fri, 02 Feb 2024 11:53:04 +0000 Donald Hunter wrote:
> > Looks like most sch's require opt. Would it be a bad idea to pull 
> > the check out to the caller? Minor simplification, plus the caller
> > has the outer message so they can use NL_SET_ERR_ATTR_MISS() and
> > friends.  
> 
> There's also these which maybe complicates things:
> 
> $ git grep -A1 'if (opt == NULL)' -- net/sched/
> net/sched/cls_flow.c:   if (opt == NULL)
> net/sched/cls_flow.c-           return -EINVAL;
> --
> net/sched/sch_choke.c:  if (opt == NULL)
> net/sched/sch_choke.c-          return -EINVAL;
> --
> net/sched/sch_fifo.c:   if (opt == NULL) {
> net/sched/sch_fifo.c-           u32 limit = qdisc_dev(sch)->tx_queue_len;
> --
> net/sched/sch_hfsc.c:   if (opt == NULL)
> net/sched/sch_hfsc.c-           return -EINVAL;
> --
> net/sched/sch_plug.c:   if (opt == NULL) {
> net/sched/sch_plug.c-           q->limit = qdisc_dev(sch)->tx_queue_len

That's fine, I was thinking opt-in. Add a bit to ops that says
"init_requires_opts" or whatnot. 

> I'm in favour of qdisc specific extack messages.

Most of them just say "$name requires options" in a more or less concise
and more or less well spelled form :( Even if we don't want to depend
purely on ATTR_MISS - extack messages support printf now, and we have
the qdisc name in the ops (ops->id), so we can printf the same string 
in the core.

Just an idea, if you all prefer to keep things as they are, that's fine.
But we've been sprinkling the damn string messages throughout TC for
years now, and still they keep coming and still if you step one foot
away from the most actively developed actions and classifiers, you're
back in the 90s :( 

Re: [PATCH 1/3] net/sched: netem: use extack

[Donald Hunter]

On Fri, 2 Feb 2024 at 16:24, Jakub Kicinski <kuba@kernel.org> wrote:
>
> That's fine, I was thinking opt-in. Add a bit to ops that says
> "init_requires_opts" or whatnot.
>
> > I'm in favour of qdisc specific extack messages.
>
> Most of them just say "$name requires options" in a more or less concise
> and more or less well spelled form :( Even if we don't want to depend
> purely on ATTR_MISS - extack messages support printf now, and we have
> the qdisc name in the ops (ops->id), so we can printf the same string
> in the core.

Fair point, it's not easy to steer people to the right options attrs.

> Just an idea, if you all prefer to keep things as they are, that's fine.
> But we've been sprinkling the damn string messages throughout TC for
> years now, and still they keep coming and still if you step one foot
> away from the most actively developed actions and classifiers, you're
> back in the 90s :(

So true. FWIW I was thinking of putting some effort into smoothing off
some of the sharp edges in all of the qdiscs, along the lines of this:

diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index ae1da08e268f..8c16fcbaad71 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -352,19 +352,28 @@ static int choke_change(struct Qdisc *sch,
struct nlattr *opt,
        if (err < 0)
                return err;

-       if (tb[TCA_CHOKE_PARMS] == NULL ||
-           tb[TCA_CHOKE_STAB] == NULL)
+       if (tb[TCA_CHOKE_PARMS] == NULL) {
+               NL_SET_ERR_MSG(extack,
+                              "Missing TCA_CHOKE_PARMS of type
'struct tc_red_qopt'");
                return -EINVAL;
+       }
+       if (tb[TCA_CHOKE_STAB] == NULL) {
+               NL_SET_ERR_MSG(extack, "Missing TCA_CHOKE_STAB");
+               return -EINVAL;
+       }

        max_P = tb[TCA_CHOKE_MAX_P] ? nla_get_u32(tb[TCA_CHOKE_MAX_P]) : 0;

        ctl = nla_data(tb[TCA_CHOKE_PARMS]);
        stab = nla_data(tb[TCA_CHOKE_STAB]);
-       if (!red_check_params(ctl->qth_min, ctl->qth_max, ctl->Wlog,
ctl->Scell_log, stab))
+       if (!red_check_params(ctl->qth_min, ctl->qth_max, ctl->Wlog,
ctl->Scell_log, stab)) {
+               NL_SET_ERR_MSG(extack, "TCA_CHOKE_PARMS has invalid
red parameters");
                return -EINVAL;
-
-       if (ctl->limit > CHOKE_MAX_QUEUE)
+       }
+       if (ctl->limit > CHOKE_MAX_QUEUE) {
+               NL_SET_ERR_MSG(extack, "TCA_CHOKE_PARMS.limit exceeds
CHOKE_MAX_QUEUE");
                return -EINVAL;
+       }

        mask = roundup_pow_of_two(ctl->limit + 1) - 1;
        if (mask != q->tab_mask) {