| Message ID | 20230530091948.1408477-2-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dced11ef84fb310f4ddfa74d1c09687b8f845d1b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xstats for tc-taprio | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next, async |
| 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: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| 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: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi, Vladimir Oltean <vladimir.oltean@nxp.com> writes: > In taprio_dump_class_stats() we don't need a reference to the root Qdisc > once we get the reference to the child corresponding to this traffic > class, so it's okay to overwrite "sch". But in a future patch we will > need the root Qdisc too, so create a dedicated "child" pointer variable > to hold the child reference. This also makes the code adhere to a more > conventional coding style. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- The patch looks good: Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> But I have a suggestion, this "taprio_queue_get() -> dev_queue->qdisc_sleeping()" dance should have the same result as calling 'taprio_leaf()'. I am thinking of using taprio_leaf() here and in taprio_dump_class(). Could be a separate commit. > net/sched/sch_taprio.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c > index 76db9a10ef50..d29e6785854d 100644 > --- a/net/sched/sch_taprio.c > +++ b/net/sched/sch_taprio.c > @@ -2388,10 +2388,10 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl, > __acquires(d->lock) > { > struct netdev_queue *dev_queue = taprio_queue_get(sch, cl); > + struct Qdisc *child = dev_queue->qdisc_sleeping; > > - sch = dev_queue->qdisc_sleeping; > - if (gnet_stats_copy_basic(d, NULL, &sch->bstats, true) < 0 || > - qdisc_qstats_copy(d, sch) < 0) > + if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 || > + qdisc_qstats_copy(d, child) < 0) > return -1; > return 0; > } > -- > 2.34.1 >
On Tue, May 30, 2023 at 02:14:17PM -0700, Vinicius Costa Gomes wrote: > But I have a suggestion, this "taprio_queue_get() -> > dev_queue->qdisc_sleeping()" dance should have the same result as > calling 'taprio_leaf()'. > > I am thinking of using taprio_leaf() here and in taprio_dump_class(). > Could be a separate commit. Got it, you want to consolidate the dev_queue->qdisc_sleeping pattern. Since taprio_dump_class() could benefit from the consolidation too, they could really be both converted separately. Or I could also handle that in this patch set, if I need to resend it.
Vladimir Oltean <vladimir.oltean@nxp.com> writes: > On Tue, May 30, 2023 at 02:14:17PM -0700, Vinicius Costa Gomes wrote: >> But I have a suggestion, this "taprio_queue_get() -> >> dev_queue->qdisc_sleeping()" dance should have the same result as >> calling 'taprio_leaf()'. >> >> I am thinking of using taprio_leaf() here and in taprio_dump_class(). >> Could be a separate commit. > > Got it, you want to consolidate the dev_queue->qdisc_sleeping pattern. > Since taprio_dump_class() could benefit from the consolidation too, they > could really be both converted separately. Or I could also handle that > in this patch set, if I need to resend it. Exactly. Both options sound great. Cheers,
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c index 76db9a10ef50..d29e6785854d 100644 --- a/net/sched/sch_taprio.c +++ b/net/sched/sch_taprio.c @@ -2388,10 +2388,10 @@ static int taprio_dump_class_stats(struct Qdisc *sch, unsigned long cl, __acquires(d->lock) { struct netdev_queue *dev_queue = taprio_queue_get(sch, cl); + struct Qdisc *child = dev_queue->qdisc_sleeping; - sch = dev_queue->qdisc_sleeping; - if (gnet_stats_copy_basic(d, NULL, &sch->bstats, true) < 0 || - qdisc_qstats_copy(d, sch) < 0) + if (gnet_stats_copy_basic(d, NULL, &child->bstats, true) < 0 || + qdisc_qstats_copy(d, child) < 0) return -1; return 0; }
In taprio_dump_class_stats() we don't need a reference to the root Qdisc once we get the reference to the child corresponding to this traffic class, so it's okay to overwrite "sch". But in a future patch we will need the root Qdisc too, so create a dedicated "child" pointer variable to hold the child reference. This also makes the code adhere to a more conventional coding style. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/sched/sch_taprio.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)