Received-SPF: pass (google.com: domain of linux-kernel+bounces-87208-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <edd520ab-b95f-4a60-a35a-2490a6d5057f@kernel.dk>
Date: Thu, 29 Feb 2024 10:49:42 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] sched/core: switch struct rq->nr_iowait to a normal
 int
Content-Language: en-US
To: Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org
Cc: peterz@infradead.org, mingo@redhat.com
References: <20240228192355.290114-1-axboe@kernel.dk>
 <20240228192355.290114-2-axboe@kernel.dk> <8734tb8b57.ffs@tglx>
 <c3abe716-3d8f-47dc-9c7d-203b05b25393@kernel.dk> <87wmqn6uaw.ffs@tglx>
From: Jens Axboe <axboe@kernel.dk>
In-Reply-To: <87wmqn6uaw.ffs@tglx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2/29/24 10:42 AM, Thomas Gleixner wrote:
> 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. ....

Will do

> 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.

But that was always racy before as well, if someone else is inc/dec'ing
->nr_iowait while it's being read, you could get either the before or
after value. This doesn't really change that. I could've sworn I
mentioned that in the commit message, but I did not.

> 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?

It was more on Peter's complaint that now we're trading a single atomic
for two, hence I got to thinking about nr_iowait in general. I don't
have numbers showing it matters, as mentioned in another email the most
costly part about this seems to be fetching task->in_iowait and not the
actual atomic.

-- 
Jens Axboe