netdev - Re: [PATCH net-next 3/4] gen_stats: Add instead Set the value in __gnet_stats_copy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211013160039.wkalyseuzz6xlf4v@linutronix.de>
Date:   Wed, 13 Oct 2021 18:00:39 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>
Subject: Re: [PATCH net-next 3/4] gen_stats: Add instead Set the value in
 __gnet_stats_copy_queue().

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