| Message ID | 20211007175000.2334713-4-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mqprio fixup and simplify code. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 7 this patch: 7 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 7 this patch: 7 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Thu, 7 Oct 2021 19:49:59 +0200 Sebastian Andrzej Siewior wrote: > --- a/net/core/gen_stats.c > +++ b/net/core/gen_stats.c > @@ -312,14 +312,14 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats, > if (cpu) { > __gnet_stats_copy_queue_cpu(qstats, cpu); > } else { > - qstats->qlen = q->qlen; > - qstats->backlog = q->backlog; > - qstats->drops = q->drops; > - qstats->requeues = q->requeues; > - qstats->overlimits = q->overlimits; > + qstats->qlen += q->qlen; > + qstats->backlog += q->backlog; > + qstats->drops += q->drops; > + qstats->requeues += q->requeues; > + qstats->overlimits += q->overlimits; > } > > - qstats->qlen = qlen; > + qstats->qlen += qlen; Looks like qlen is going to be added twice for the non-per-cpu case?
On 2021-10-08 16:38:51 [-0700], Jakub Kicinski wrote: > On Thu, 7 Oct 2021 19:49:59 +0200 Sebastian Andrzej Siewior wrote: > > --- a/net/core/gen_stats.c > > +++ b/net/core/gen_stats.c > > @@ -312,14 +312,14 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats, > > if (cpu) { > > __gnet_stats_copy_queue_cpu(qstats, cpu); > > } else { > > - qstats->qlen = q->qlen; > > - qstats->backlog = q->backlog; > > - qstats->drops = q->drops; > > - qstats->requeues = q->requeues; > > - qstats->overlimits = q->overlimits; > > + qstats->qlen += q->qlen; > > + qstats->backlog += q->backlog; > > + qstats->drops += q->drops; > > + qstats->requeues += q->requeues; > > + qstats->overlimits += q->overlimits; > > } > > > > - qstats->qlen = qlen; > > + qstats->qlen += qlen; > > Looks like qlen is going to be added twice for the non-per-cpu case? Hmmm. Let me dive into unknown territory… Yes, it does. Also in the pcpu-case it does not look very straight what goes on: qlen = qdisc_qlen_sum(qdisc); /* sum of per-CPU cpu_qstat.qlen */ __gnet_stats_copy_queue() /* sch.qstats.qlen = qlen */ sch->q.qlen += qlen; sch->qstats.qlen is qdisc_qlen_sum() of the qdisc from last for loop. sch->q.qlen contains qdisc_qlen_sum() of all qdiscs from the for loop. I doubt this is intended. My guess is that a sum (like in the !pcpu case is intended). We have - qdisc.q.qlen qdisc_skb_head::qlen - qdisc.qstats.qlen aka gnet_stats_queue::qlen. qdisc_skb_head::qlen is incremented if skbs are added to the qdisc which are about to be sent. Usually the skb is added to the qdisc_skb_head but some scheduling classes have their own queues (say sch_sfq) and probably increment this field just to keep things like qdisc_is_empty() working. gnet_stats_queue::qlen is the number of skbs either in Qdisc::skb_bad_txq or Qdisc::gso_skb. qdisc_update_stats_at_enqueue() increments in percpu case the per-CPU gnet_stats_queue::qlen but in the !percpu case it increments qdisc_skb_head::qlen. But there is the qstats member which is also if type gnet_stats_queue. For backlog, the gnet_stats_queue struct is always used, either per-CPU or the global one. Not here. Not sure if it on purpose or not. The same true for qdisc_enqueue_skb_bad_txq() and dev_requeue_skb() plus their counterpart so it consistent. This brings me to __gnet_stats_copy_queue(). In the percpu case, caller's gnet_stats_queue::qlen is set to 0 multiple times. And then caller's qlen argument is assigned to the qlen member. In !percpu case it copies gnet_stats_queue::qlen. But I don't see an increment/decrement of that field here so it has to be zero. Also at the end it sets the field to caller's qlen. So… This probably works because callers of __gnet_stats_copy_queue() invoke qdisc_qlen_sum() which returns the sum of: qstats::qlen (0) + per-CPU qstats::qlen || !per-CPU qdisc_skb_head::qlen So in the end the caller figures out qlen is and passes it as a argument. The copy process of qlen ist decoy. But then there is gnet_stats_copy_queue(). sch_fq_codel modifies qdisc_skb_head::qlen on fq_codel_enqueue()/ fq_codel_dequeue(). But fq_codel_dump_class_stats() returns statistics for a specific flow so it counts the number of skb which is less than the value qdisc_skb_head.qlen if multiple flows are configured. So I think I could remove the qlen argument from __gnet_stats_copy_queue() and just copy what is there. And make a real copy, not just summing all qlen into qdisc_skb_head.qlen and leaving gnet_stats_queue.qlen with the last value. Sebastian
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c index e12e544a7ab0f..76dbae98c83fd 100644 --- a/net/core/gen_stats.c +++ b/net/core/gen_stats.c @@ -312,14 +312,14 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats, if (cpu) { __gnet_stats_copy_queue_cpu(qstats, cpu); } else { - qstats->qlen = q->qlen; - qstats->backlog = q->backlog; - qstats->drops = q->drops; - qstats->requeues = q->requeues; - qstats->overlimits = q->overlimits; + qstats->qlen += q->qlen; + qstats->backlog += q->backlog; + qstats->drops += q->drops; + qstats->requeues += q->requeues; + qstats->overlimits += q->overlimits; } - qstats->qlen = qlen; + qstats->qlen += qlen; } EXPORT_SYMBOL(__gnet_stats_copy_queue);
Based on review there are five users of __gnet_stats_copy_queue as of today: - qdisc_qstats_qlen_backlog(), gnet_stats_copy_queue(), memsets() bstats to zero, single invocation. - mq_dump(), mqprio_dump(), mqprio_dump_class_stats(), memsets() bstats to zero, multiple invocation but does not use the function due to !qdisc_is_percpu_stats(). It will probably simplify in percpu stats case if the value would be added and not just stored. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- net/core/gen_stats.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)