[PATCH net-next v2 0/4] net/sched: cleanup parsing prints in htb and qfq

[PATCH net-next v2 0/4] net/sched: cleanup parsing prints in htb and qfq

[Pedro Tammela]

These two qdiscs are still using prints on dmesg to report parsing
errors. Since the parsing code has access to extack, convert these error
messages to extack.

QFQ also had the opportunity to remove some redundant code in the
parameters parsing by transforming some attributes into parsing
policies.

v1->v2: Address suggestions by Jakub

Pedro Tammela (4):
  net/sched: sch_htb: use extack on errors messages
  net/sched: sch_qfq: use extack on errors messages
  net/sched: sch_qfq: refactor parsing of netlink parameters
  selftests: tc-testing: add more tests for sch_qfq

 net/sched/sch_htb.c                           | 17 ++---
 net/sched/sch_qfq.c                           | 37 +++++-----
 .../tc-testing/tc-tests/qdiscs/qfq.json       | 72 +++++++++++++++++++
 3 files changed, 99 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors messages

[Pedro Tammela]

Some error messages are still being printed to dmesg.
Since extack is available, provide error messages there.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_htb.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 92f2975b6a82..79f5c4454fc3 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1786,7 +1786,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 		goto failure;
 
 	err = nla_parse_nested_deprecated(tb, TCA_HTB_MAX, opt, htb_policy,
-					  NULL);
+					  extack);
 	if (err < 0)
 		goto failure;
 
@@ -1858,7 +1858,7 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 
 		/* check maximal depth */
 		if (parent && parent->parent && parent->parent->level < 2) {
-			pr_err("htb: tree is too deep\n");
+			NL_SET_ERR_MSG_MOD(extack, "tree is too deep");
 			goto failure;
 		}
 		err = -ENOBUFS;
@@ -1917,8 +1917,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 			};
 			err = htb_offload(dev, &offload_opt);
 			if (err) {
-				pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
-				       err);
+				NL_SET_ERR_MSG(extack,
+					       "Failed to offload leaf alloc queue");
 				goto err_kill_estimator;
 			}
 			dev_queue = netdev_get_tx_queue(dev, offload_opt.qid);
@@ -1937,8 +1937,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 			};
 			err = htb_offload(dev, &offload_opt);
 			if (err) {
-				pr_err("htb: TC_HTB_LEAF_TO_INNER failed with err = %d\n",
-				       err);
+				NL_SET_ERR_MSG(extack,
+					       "Failed to offload leaf to inner");
 				htb_graft_helper(dev_queue, old_q);
 				goto err_kill_estimator;
 			}
@@ -2067,8 +2067,9 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
 	qdisc_put(parent_qdisc);
 
 	if (warn)
-		pr_warn("HTB: quantum of class %X is %s. Consider r2q change.\n",
-			    cl->common.classid, (warn == -1 ? "small" : "big"));
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "quantum of class %X is %s. Consider r2q change.",
+				       cl->common.classid, (warn == -1 ? "small" : "big"));
 
 	qdisc_class_hash_grow(sch, &q->clhash);
 
-- 
2.34.1

Re: [PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors messages

[Simon Horman]

On Mon, Apr 17, 2023 at 02:12:15PM -0300, Pedro Tammela wrote:
> Some error messages are still being printed to dmesg.
> Since extack is available, provide error messages there.
> 
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors messages

[Jakub Kicinski]

On Mon, 17 Apr 2023 14:12:15 -0300 Pedro Tammela wrote:
> @@ -1917,8 +1917,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>  			};
>  			err = htb_offload(dev, &offload_opt);
>  			if (err) {
> -				pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
> -				       err);
> +				NL_SET_ERR_MSG(extack,
> +					       "Failed to offload leaf alloc queue");
>  				goto err_kill_estimator;
>  			}
>  			dev_queue = netdev_get_tx_queue(dev, offload_opt.qid);
> @@ -1937,8 +1937,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>  			};
>  			err = htb_offload(dev, &offload_opt);
>  			if (err) {
> -				pr_err("htb: TC_HTB_LEAF_TO_INNER failed with err = %d\n",
> -				       err);
> +				NL_SET_ERR_MSG(extack,
> +					       "Failed to offload leaf to inner");

I missed the message changes on v1, but since the patches are already
Changes Requested in patchwork...

IDK what TC_HTB_LEAF_TO_INNER is exactly, neither do I understand
"Failed to offload leaf to inner". The first one should be "Failed to
alloc offload leaf queue" if anything.

Let's just stick to the existing messages?

Re: [PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors messages

[Pedro Tammela]

On 19/04/2023 22:08, Jakub Kicinski wrote:
> On Mon, 17 Apr 2023 14:12:15 -0300 Pedro Tammela wrote:
>> @@ -1917,8 +1917,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>   			};
>>   			err = htb_offload(dev, &offload_opt);
>>   			if (err) {
>> -				pr_err("htb: TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n",
>> -				       err);
>> +				NL_SET_ERR_MSG(extack,
>> +					       "Failed to offload leaf alloc queue");
>>   				goto err_kill_estimator;
>>   			}
>>   			dev_queue = netdev_get_tx_queue(dev, offload_opt.qid);
>> @@ -1937,8 +1937,8 @@ static int htb_change_class(struct Qdisc *sch, u32 classid,
>>   			};
>>   			err = htb_offload(dev, &offload_opt);
>>   			if (err) {
>> -				pr_err("htb: TC_HTB_LEAF_TO_INNER failed with err = %d\n",
>> -				       err);
>> +				NL_SET_ERR_MSG(extack,
>> +					       "Failed to offload leaf to inner");
> 
> I missed the message changes on v1, but since the patches are already
> Changes Requested in patchwork...
> 
> IDK what TC_HTB_LEAF_TO_INNER is exactly, neither do I understand
> "Failed to offload leaf to inner". The first one should be "Failed to
> alloc offload leaf queue" if anything.
> 
> Let's just stick to the existing messages?

The existing messages omit that the offload operation failed, which I 
think it would be important to say in these two cases.
But taking a second look, the driver gets the extack so it should be up 
to the driver to provide the appropriate context when the offload fails. 
What do you think about demoting these messages to WEAK and change them 
to something like:
"Failed to offload %attribute"

Re: [PATCH net-next v2 1/4] net/sched: sch_htb: use extack on errors messages

[Jakub Kicinski]

On Wed, 19 Apr 2023 23:14:54 -0300 Pedro Tammela wrote:
> > I missed the message changes on v1, but since the patches are already
> > Changes Requested in patchwork...
> > 
> > IDK what TC_HTB_LEAF_TO_INNER is exactly, neither do I understand
> > "Failed to offload leaf to inner". The first one should be "Failed to
> > alloc offload leaf queue" if anything.
> > 
> > Let's just stick to the existing messages?  
> 
> The existing messages omit that the offload operation failed, which I 
> think it would be important to say in these two cases.
> But taking a second look, the driver gets the extack so it should be up 
> to the driver to provide the appropriate context when the offload fails. 
> What do you think about demoting these messages to WEAK and change them 
> to something like:
> "Failed to offload %attribute"

Good idea!

[PATCH net-next v2 2/4] net/sched: sch_qfq: use extack on errors messages

[Pedro Tammela]

Some error messages are still being printed to dmesg.
Since extack is available, provide error messages there.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_qfq.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index cf5ebe43b3b4..323609cfbc67 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -402,8 +402,8 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	int err;
 	int delta_w;
 
-	if (tca[TCA_OPTIONS] == NULL) {
-		pr_notice("qfq: no options\n");
+	if (NL_REQ_ATTR_CHECK(extack, NULL, tca, TCA_OPTIONS)) {
+		NL_SET_ERR_MSG_MOD(extack, "missing options");
 		return -EINVAL;
 	}
 
@@ -441,8 +441,9 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	delta_w = weight - (cl ? cl->agg->class_weight : 0);
 
 	if (q->wsum + delta_w > QFQ_MAX_WSUM) {
-		pr_notice("qfq: total weight out of range (%d + %u)\n",
-			  delta_w, q->wsum);
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "total weight out of range (%d + %u)\n",
+				       delta_w, q->wsum);
 		return -EINVAL;
 	}
 
-- 
2.34.1

Re: [PATCH net-next v2 2/4] net/sched: sch_qfq: use extack on errors messages

[Jamal Hadi Salim]

On Mon, Apr 17, 2023 at 1:12 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>
> Some error messages are still being printed to dmesg.
> Since extack is available, provide error messages there.
>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

> ---
>  net/sched/sch_qfq.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index cf5ebe43b3b4..323609cfbc67 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c
> @@ -402,8 +402,8 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>         int err;
>         int delta_w;
>
> -       if (tca[TCA_OPTIONS] == NULL) {
> -               pr_notice("qfq: no options\n");
> +       if (NL_REQ_ATTR_CHECK(extack, NULL, tca, TCA_OPTIONS)) {
> +               NL_SET_ERR_MSG_MOD(extack, "missing options");
>                 return -EINVAL;
>         }
>
> @@ -441,8 +441,9 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>         delta_w = weight - (cl ? cl->agg->class_weight : 0);
>
>         if (q->wsum + delta_w > QFQ_MAX_WSUM) {
> -               pr_notice("qfq: total weight out of range (%d + %u)\n",
> -                         delta_w, q->wsum);
> +               NL_SET_ERR_MSG_FMT_MOD(extack,
> +                                      "total weight out of range (%d + %u)\n",
> +                                      delta_w, q->wsum);
>                 return -EINVAL;
>         }
>
> --
> 2.34.1
>

Re: [PATCH net-next v2 2/4] net/sched: sch_qfq: use extack on errors messages

[Simon Horman]

On Mon, Apr 17, 2023 at 02:12:16PM -0300, Pedro Tammela wrote:
> Some error messages are still being printed to dmesg.
> Since extack is available, provide error messages there.
> 
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next v2 3/4] net/sched: sch_qfq: refactor parsing of netlink parameters

[Pedro Tammela]

Two parameters can be transformed into netlink policies and
validated while parsing the netlink message.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 net/sched/sch_qfq.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 323609cfbc67..151102ac8cce 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -214,9 +214,15 @@ static struct qfq_class *qfq_find_class(struct Qdisc *sch, u32 classid)
 	return container_of(clc, struct qfq_class, common);
 }
 
+static struct netlink_range_validation lmax_range = {
+	.min = QFQ_MIN_LMAX,
+	.max = (1UL << QFQ_MTU_SHIFT),
+};
+
 static const struct nla_policy qfq_policy[TCA_QFQ_MAX + 1] = {
-	[TCA_QFQ_WEIGHT] = { .type = NLA_U32 },
-	[TCA_QFQ_LMAX] = { .type = NLA_U32 },
+	[TCA_QFQ_WEIGHT] =
+		NLA_POLICY_RANGE(NLA_U32, 1, (1UL << QFQ_MAX_WSHIFT)),
+	[TCA_QFQ_LMAX] = NLA_POLICY_FULL_RANGE(NLA_U32, &lmax_range),
 };
 
 /*
@@ -408,26 +414,18 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	}
 
 	err = nla_parse_nested_deprecated(tb, TCA_QFQ_MAX, tca[TCA_OPTIONS],
-					  qfq_policy, NULL);
+					  qfq_policy, extack);
 	if (err < 0)
 		return err;
 
-	if (tb[TCA_QFQ_WEIGHT]) {
+	if (tb[TCA_QFQ_WEIGHT])
 		weight = nla_get_u32(tb[TCA_QFQ_WEIGHT]);
-		if (!weight || weight > (1UL << QFQ_MAX_WSHIFT)) {
-			pr_notice("qfq: invalid weight %u\n", weight);
-			return -EINVAL;
-		}
-	} else
+	else
 		weight = 1;
 
-	if (tb[TCA_QFQ_LMAX]) {
+	if (tb[TCA_QFQ_LMAX])
 		lmax = nla_get_u32(tb[TCA_QFQ_LMAX]);
-		if (lmax < QFQ_MIN_LMAX || lmax > (1UL << QFQ_MTU_SHIFT)) {
-			pr_notice("qfq: invalid max length %u\n", lmax);
-			return -EINVAL;
-		}
-	} else
+	else
 		lmax = psched_mtu(qdisc_dev(sch));
 
 	inv_w = ONE_FP / weight;
-- 
2.34.1

Re: [PATCH net-next v2 3/4] net/sched: sch_qfq: refactor parsing of netlink parameters

[Simon Horman]

On Mon, Apr 17, 2023 at 02:12:17PM -0300, Pedro Tammela wrote:
> Two parameters can be transformed into netlink policies and
> validated while parsing the netlink message.
> 
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

> ---
>  net/sched/sch_qfq.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
> index 323609cfbc67..151102ac8cce 100644
> --- a/net/sched/sch_qfq.c
> +++ b/net/sched/sch_qfq.c
> @@ -214,9 +214,15 @@ static struct qfq_class *qfq_find_class(struct Qdisc *sch, u32 classid)
>  	return container_of(clc, struct qfq_class, common);
>  }
>  
> +static struct netlink_range_validation lmax_range = {
> +	.min = QFQ_MIN_LMAX,
> +	.max = (1UL << QFQ_MTU_SHIFT),
> +};
> +
>  static const struct nla_policy qfq_policy[TCA_QFQ_MAX + 1] = {
> -	[TCA_QFQ_WEIGHT] = { .type = NLA_U32 },
> -	[TCA_QFQ_LMAX] = { .type = NLA_U32 },
> +	[TCA_QFQ_WEIGHT] =
> +		NLA_POLICY_RANGE(NLA_U32, 1, (1UL << QFQ_MAX_WSHIFT)),
> +	[TCA_QFQ_LMAX] = NLA_POLICY_FULL_RANGE(NLA_U32, &lmax_range),
>  };

Maybe someday we can teach this file about the BIT macro.

...

[PATCH net-next v2 4/4] selftests: tc-testing: add more tests for sch_qfq

[Pedro Tammela]

The QFQ qdisc class has parameter bounds that are not being
checked for correctness.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 .../tc-testing/tc-tests/qdiscs/qfq.json       | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/qfq.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/qfq.json
index 330f1a25e0ab..147899a868d3 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/qfq.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/qfq.json
@@ -46,6 +46,30 @@
             "$IP link del dev $DUMMY type dummy"
         ]
     },
+    {
+        "id": "d364",
+        "name": "Test QFQ with max class weight setting",
+        "category": [
+            "qdisc",
+            "qfq"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true",
+            "$TC qdisc add dev $DUMMY handle 1: root qfq"
+        ],
+        "cmdUnderTest": "$TC class add dev $DUMMY parent 1: classid 1:1 qfq weight 9999",
+        "expExitCode": "2",
+        "verifyCmd": "$TC class show dev $DUMMY",
+        "matchPattern": "class qfq 1:1 root weight 9999 maxpkt",
+        "matchCount": "0",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
     {
         "id": "8452",
         "name": "Create QFQ with class maxpkt setting",
@@ -70,6 +94,54 @@
             "$IP link del dev $DUMMY type dummy"
         ]
     },
+    {
+        "id": "22df",
+        "name": "Test QFQ class maxpkt setting lower bound",
+        "category": [
+            "qdisc",
+            "qfq"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true",
+            "$TC qdisc add dev $DUMMY handle 1: root qfq"
+        ],
+        "cmdUnderTest": "$TC class add dev $DUMMY parent 1: classid 1:1 qfq maxpkt 128",
+        "expExitCode": "2",
+        "verifyCmd": "$TC class show dev $DUMMY",
+        "matchPattern": "class qfq 1:1 root weight 1 maxpkt 128",
+        "matchCount": "0",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
+    {
+        "id": "92ee",
+        "name": "Test QFQ class maxpkt setting upper bound",
+        "category": [
+            "qdisc",
+            "qfq"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link add dev $DUMMY type dummy || /bin/true",
+            "$TC qdisc add dev $DUMMY handle 1: root qfq"
+        ],
+        "cmdUnderTest": "$TC class add dev $DUMMY parent 1: classid 1:1 qfq maxpkt 99999",
+        "expExitCode": "2",
+        "verifyCmd": "$TC class show dev $DUMMY",
+        "matchPattern": "class qfq 1:1 root weight 1 maxpkt 99999",
+        "matchCount": "0",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1: root",
+            "$IP link del dev $DUMMY type dummy"
+        ]
+    },
     {
         "id": "d920",
         "name": "Create QFQ with multiple class setting",
-- 
2.34.1