Re: [PATCH 2/2] block: cache current nsec time in struct blk_plug

[Jens Axboe <axboe@kernel.dk>]

On 1/16/24 2:51 AM, Kanchan Joshi wrote:
> On 1/16/2024 3:23 AM, Jens Axboe wrote:
> 
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 11342af420d0..cc4db4d92c75 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1073,6 +1073,7 @@ void blk_start_plug_nr_ios(struct blk_plug *plug, unsigned short nr_ios)
>>   	if (tsk->plug)
>>   		return;
>>   
>> +	plug->cur_ktime = 0;
>>   	plug->mq_list = NULL;
>>   	plug->cached_rq = NULL;
>>   	plug->nr_ios = min_t(unsigned short, nr_ios, BLK_MAX_REQUEST_COUNT);
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index 2f9ceea0e23b..23c237b22071 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -942,6 +942,7 @@ struct blk_plug {
>>   
>>   	/* if ios_left is > 1, we can batch tag/rq allocations */
>>   	struct request *cached_rq;
>> +	u64 cur_ktime;
>>   	unsigned short nr_ios;
>>   
>>   	unsigned short rq_count;
>> @@ -977,7 +978,15 @@ long nr_blockdev_pages(void);
>>   
>>   static inline u64 blk_time_get_ns(void)
>>   {
>> -	return ktime_get_ns();
>> +	struct blk_plug *plug = current->plug;
>> +
>> +	if (!plug)
>> +		return ktime_get_ns();
>> +	if (!(plug->cur_ktime & 1ULL)) {
>> +		plug->cur_ktime = ktime_get_ns();
>> +		plug->cur_ktime |= 1ULL;
>> +	}
>> +	return plug->cur_ktime;
> 
> I did not understand the relevance of 1ULL here. If ktime_get_ns()
> returns even value, it will turn that into an odd value before
> caching.

Right, it's potentially round it up by 1 nsec.

> And that value will be returned for the subsequent calls. But
> how is that better compared to just caching whatever ktime_get_ns()
> returned.

0 could be a valid time. You could argue that this doesn't matter, we'd
just do an extra ktime_get_ns() in that case. And that's probably fine.
The LSB was meant to indicate "time stamp is valid".

But not that important imho, I'll either add a comment or just use 0 as
both the initializer (as it is now) and non-zero to indicate if the
timestamp is valid or not.

-- 
Jens Axboe