| Message ID | 20230414185309.220286-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 |
| 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 |
On Fri, 14 Apr 2023 15:53:09 -0300 Pedro Tammela wrote: > @@ -1917,8 +1917,9 @@ 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_FMT_MOD(extack, What's the ruling on using _MOD() in qdiscs ? There are some extacks already in this file without _MOD(). > + "TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n", > + err); The formatting of an error into the message is unnecessary duplication. The error value does not make it to dmesg so we need to print it there, but it's already present at the netlink level.
On Fri, Apr 14, 2023 at 9:13 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 14 Apr 2023 15:53:09 -0300 Pedro Tammela wrote: > > @@ -1917,8 +1917,9 @@ 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_FMT_MOD(extack, > > What's the ruling on using _MOD() in qdiscs ? > There are some extacks already in this file without _MOD(). There is no "rule" other than the LinuxWay(tm) i.e. people cutnpaste. It's not just on qdiscs that this inconsistency exists but also on filters and actions. Do we need a rule to prefer one over the other? _MOD() seems to provide more information - which is always useful. cheers, jamal
On Sat, 15 Apr 2023 10:06:36 -0400 Jamal Hadi Salim wrote: > There is no "rule" other than the LinuxWay(tm) i.e. people cutnpaste. :-) > It's not just on qdiscs that this inconsistency exists but also on > filters and actions. > Do we need a rule to prefer one over the other? _MOD() seems to > provide more information - which is always useful. People started adding _MOD() on every extack so I was pushing back a little lately. It will bloat the strings and makes the output between parsing and hand coded checks different. Plus if we really want the mod name, we should just make it the default and remove the non-mod version. The rule of thumb I had was that if the message comes from "core" of a family then _MOD() is unnecessary. In this case HTB is a one-of-many implementations so _MOD() seems fine. Then again errors about parsing TCA_HTB_* attrs are unlikely to come from something else than HTB..
On 17/04/2023 12:52, Jakub Kicinski wrote: > [...] > > The rule of thumb I had was that if the message comes from "core" of > a family then _MOD() is unnecessary. In this case HTB is a one-of-many > implementations so _MOD() seems fine. Then again errors about parsing > TCA_HTB_* attrs are unlikely to come from something else than HTB.. Agreed, will re-spin with this in mind. Pedro
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 92f2975b6a82..bc2da8027650 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,9 @@ 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_FMT_MOD(extack, + "TC_HTB_LEAF_ALLOC_QUEUE failed with err = %d\n", + err); goto err_kill_estimator; } dev_queue = netdev_get_tx_queue(dev, offload_opt.qid); @@ -1937,8 +1938,9 @@ 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_FMT_MOD(extack, + "TC_HTB_LEAF_TO_INNER failed with err = %d", + err); htb_graft_helper(dev_queue, old_q); goto err_kill_estimator; } @@ -2067,8 +2069,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);