diff mbox series

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

Message ID 20230417171218.333567-2-pctammela@mojatatu.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net/sched: cleanup parsing prints in htb and qfq | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch warning WARNING: line length of 91 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pedro Tammela April 17, 2023, 5:12 p.m. UTC 
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(-)

Comments

Simon Horman April 19, 2023, 12:51 p.m. UTC | #1 
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>
Jakub Kicinski April 20, 2023, 1:08 a.m. UTC | #2 
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?
Pedro Tammela April 20, 2023, 2:14 a.m. UTC | #3 
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"
Jakub Kicinski April 20, 2023, 2:17 p.m. UTC | #4 
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!
diff mbox series

Patch

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);