linux-kernel - Re: [PATCH 1/2] sched/core: switch struct rq->nr

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: <87wmqn6uaw.ffs@tglx>
Date: Thu, 29 Feb 2024 18:42:47 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org
Cc: peterz@...radead.org, mingo@...hat.com
Subject: Re: [PATCH 1/2] sched/core: switch struct rq->nr_iowait to a normal
 int

On Thu, Feb 29 2024 at 10:19, Jens Axboe wrote:
> On 2/29/24 9:53 AM, Thomas Gleixner wrote:
>> On Wed, Feb 28 2024 at 12:16, Jens Axboe wrote:
>>> In 3 of the 4 spots where we modify rq->nr_iowait we already hold the
>> 
>> We modify something and hold locks? It's documented that changelogs
>> should not impersonate code. It simply does not make any sense.
>
> Agree it doesn't read that well... It's meant to say that we already
> hold the rq lock in 3 of the 4 spots, hence using atomic_inc/dec is
> pointless for those cases.

That and the 'we'. Write it neutral.

The accounting of rq::nr_iowait is using an atomic_t but 3 out of 4
places hold runqueue lock already. ....

So but I just noticed that there is actually an issue with this:

>  unsigned int nr_iowait_cpu(int cpu)
>  {
> -	return atomic_read(&cpu_rq(cpu)->nr_iowait);
> +	struct rq *rq = cpu_rq(cpu);
> +
> +	return rq->nr_iowait - atomic_read(&rq->nr_iowait_remote);

The access to rq->nr_iowait is not protected by the runqueue lock and
therefore a data race when @cpu is not the current CPU.

This needs to be properly annotated and explained why it does not
matter.

So s/Reviewed-by/Un-Reviewed-by/

Though thinking about it some more. Is this split a real benefit over
always using the atomic? Do you have numbers to show?

Thanks,

        tglx