Received-SPF: pass (google.com: domain of linux-kernel+bounces-147010-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <29b6f42f-6917-4584-a53a-4711754c6fcf@arm.com>
Date: Tue, 16 Apr 2024 15:25:26 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/4] sched/core: split iowait state into two states
From: Christian Loehle <christian.loehle@arm.com>
To: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org
Cc: peterz@infradead.org, tglx@linutronix.de
References: <20240416121526.67022-1-axboe@kernel.dk>
 <20240416121526.67022-5-axboe@kernel.dk>
 <f1685b30-9d0d-419e-b004-590edf21315a@arm.com>
Content-Language: en-US
In-Reply-To: <f1685b30-9d0d-419e-b004-590edf21315a@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 16/04/2024 15:10, Christian Loehle wrote:
>>[snip]
>> +	atomic_long_add(val, &task_rq(p)->nr_iowait);
>>  #else
>> -	atomic_inc(&task_rq(p)->nr_iowait);
>> +	int val = 1 + ((int) p->in_iowait_acct << 16);
>> +	atomic_add(val, &task_rq(p)->nr_iowait);
>>  #endif
>>  }
>>  
>>  static void task_iowait_dec(struct task_struct *p)
>>  {
>>  #ifdef CONFIG_64BIT
>> -	atomic_long_dec(&task_rq(p)->nr_iowait);
>> +	long val = 1 + ((long) p->in_iowait_acct << 32);
>> +	atomic_long_sub(val, &task_rq(p)->nr_iowait);
>>  #else
>> -	atomic_dec(&task_rq(p)->nr_iowait);
>> +	int val = 1 + ((int) p->in_iowait_acct << 16);
>> +	atomic_sub(val, &task_rq(p)->nr_iowait);
>>  #endif
>>  }
>>  
>>  int rq_iowait(struct rq *rq)
>>  {
>>  #ifdef CONFIG_64BIT
>> -	return atomic_long_read(&rq->nr_iowait);
>> +	return atomic_long_read(&rq->nr_iowait) & ((1UL << 32) - 1);
>> +#else
>> +	return atomic_read(&rq->nr_iowait) & ((1U << 16) - 1);
>> +#endif
>> +}
>> +
>> +int rq_iowait_acct(struct rq *rq)
>> +{
>> +#ifdef CONFIG_64BIT
>> +	return atomic_long_read(&rq->nr_iowait) >> 32;
>>  #else
>> -	return atomic_read(&rq->nr_iowait);
>> +	return atomic_read(&rq->nr_iowait) >> 16;
>>  #endif
>>  }
>>  
>> @@ -5497,7 +5514,12 @@ unsigned long long nr_context_switches(void)
>>   * it does become runnable.
>>   */
>>  
>> -unsigned int nr_iowait_cpu(int cpu)
>> +unsigned int nr_iowait_acct_cpu(int cpu)
> 
> There is a comment above here by Daniel referring to "these two
> interfaces [...]", originally referring to nr_iowait_cpu() and
> get_iowait_load().
> get_iowait_load() was removed in commit a7fe5190c03f ("cpuidle: menu: Remove get_loadavg() from the performance multiplier")
> but nr_iowait_cpu() remains, so the comment should remain above it.
> Rewording is also way overdue, although that's hardly on your
> patch ;)
> 
FWIW, but also feel free to reword yourself.

From ba66fe24c64f5615b9820b0fe24857ecbf18fc5f Mon Sep 17 00:00:00 2001
From: Christian Loehle <christian.loehle@arm.com>
Date: Tue, 16 Apr 2024 15:20:22 +0100
Subject: [PATCH] sched/core: Reword comment about iowait interface

The original comment referenced an interface which was removed in
the meantime by commit a7fe5190c03f ("cpuidle: menu: Remove
get_loadavg() from the performance multiplier").

Furthermore the function has moved so move the comment accordingly.

Signed-off-by: Christian Loehle <christian.loehle@arm.com>
---
 kernel/sched/core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d52d3118dd73..bb10b9bc37d6 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5507,18 +5507,19 @@ unsigned long long nr_context_switches(void)
 	return sum;
 }
 
-/*
- * Consumers of these two interfaces, like for example the cpuidle menu
- * governor, are using nonsensical data. Preferring shallow idle state selection
- * for a CPU that has IO-wait which might not even end up running the task when
- * it does become runnable.
- */
 
 unsigned int nr_iowait_acct_cpu(int cpu)
 {
 	return rq_iowait_acct(cpu_rq(cpu));
 }
 
+/*
+ * Consumers of this interface like the cpuidle menu governor are using
+ * nonsensical data. Preferring shallow idle state selection for a CPU that
+ * has IO-wait which might not even end up being selected for the task again
+ * when it does become runnable.
+ */
+
 unsigned nr_iowait_cpu(int cpu)
 {
 	return rq_iowait(cpu_rq(cpu));
-- 
2.34.1