Received-SPF: pass (google.com: domain of linux-kernel+bounces-87206-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: <c842c819-7035-45b1-8098-55a2d24a7d47@kernel.dk>
Date: Thu, 29 Feb 2024 10:45:40 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/2] sched/core: split iowait state into two states
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-3-axboe@kernel.dk> <87zfvj6uub.ffs@tglx>
From: Jens Axboe <axboe@kernel.dk>
In-Reply-To: <87zfvj6uub.ffs@tglx>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2/29/24 10:31 AM, Thomas Gleixner wrote:
> On Wed, Feb 28 2024 at 12:16, Jens Axboe wrote:
>> iowait is a bogus metric, but it's helpful in the sense that it allows
>> short waits to not enter sleep states that have a higher exit latency
>> than we would've picked for iowait'ing tasks. However, it's harmless in
>> that lots of applications and monitoring assumes that iowait is busy
>> time, or otherwise use it as a health metric. Particularly for async
>> IO it's entirely nonsensical.
>>
>> Split the iowait part into two parts - one that tracks whether we need
>> boosting for short waits, and one that says we need to account the
>> task
> 
> We :)

I appreciate the commit message police :-)

I'll rewrite it.

>> +/*
>> + * Returns a token which is comprised of the two bits of iowait wait state -
>> + * one is whether we're making ourselves as in iowait for cpufreq reasons,
>> + * and the other is if the task should be accounted as such.
>> + */
>>  int io_schedule_prepare(void)
>>  {
>> -	int old_iowait = current->in_iowait;
>> +	int old_wait_flags = 0;
>> +
>> +	if (current->in_iowait)
>> +		old_wait_flags |= TASK_IOWAIT;
>> +	if (current->in_iowait_acct)
>> +		old_wait_flags |= TASK_IOWAIT_ACCT;
>>  
>>  	current->in_iowait = 1;
>> +	current->in_iowait_acct = 1;
>>  	blk_flush_plug(current->plug, true);
>> -	return old_iowait;
>> +	return old_wait_flags;
>>  }
>>  
>> -void io_schedule_finish(int token)
>> +void io_schedule_finish(int old_wait_flags)
>>  {
>> -	current->in_iowait = token;
>> +	if (!(old_wait_flags & TASK_IOWAIT))
>> +		current->in_iowait = 0;
>> +	if (!(old_wait_flags & TASK_IOWAIT_ACCT))
>> +		current->in_iowait_acct = 0;
> 
> Why? TASK_IOWAIT_ACCT requires TASK_IOWAIT, right? So if TASK_IOWAIT was
> not set then TASK_IOWAIT_ACCT must have been clear too, no?

It does, IOWAIT_ACCT always nests inside IOWAIT. I guess it would be
more explanatory as:

	/*
	 * If TASK_IOWAIT isn't set, then TASK_IOWAIT_ACCT cannot have
	 * been set either as it nests inside TASK_IOWAIT.
	 */
	if (!(old_wait_flags & TASK_IOWAIT))
		current->in_iowait = 0;
	else if (!(old_wait_flags & TASK_IOWAIT_ACCT))
		current->in_iowait_acct = 0;

?

-- 
Jens Axboe