Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH 1/2] sched/core: switch struct rq->nr_iowait to a normal int | Date | Thu, 29 Feb 2024 18:42:47 +0100 |
| |
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
| |