linux-kernel - Re: [PATCH v2 3/3] blk-mq: fix start_time_ns and alloc_time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e1da325b-8f16-f4d2-d6bf-65976a4239b4@linux.dev>
Date:   Tue, 27 Jun 2023 19:32:42 +0800
From:   Chengming Zhou <chengming.zhou@...ux.dev>
To:     Tejun Heo <tj@...nel.org>
Cc:     axboe@...nel.dk, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, zhouchengming@...edance.com
Subject: Re: [PATCH v2 3/3] blk-mq: fix start_time_ns and alloc_time_ns for
 pre-allocated rq

On 2023/6/27 04:46, Tejun Heo wrote:
> Hello,
> 
> I only glanced the blk-mq core part but in general this looks a lot better
> than the previous one.

Thanks for your review!

> 
> On Mon, Jun 26, 2023 at 01:04:05PM +0800, chengming.zhou@...ux.dev wrote:
>> Note we don't skip setting alloc_time_ns and start_time_ns for all
>> pre-allocated rq, since the first returned rq still need to be set.
> 
> This part is a bit curious for me tho. Why do we need to set it at batch
> allocation time and then at actual dispensing from the bat later? Who uses
> the alloc time stamp inbetween?
> 

Yes, this part should need more explanation, and my explanation is not clear.

Now the batched pre-allocation code looks like this:

```
if (!rq_list_empty(plug->cached_rq))

  get pre-allocated rq from plug cache

  // we set alloc and start time here
  return rq

else
  rq = __blk_mq_alloc_requests_batch() do batched allocation (1)
  (2)
  return rq
```

In (1) we alloc some requests and push them in plug list, and pop one request
to return to use. So this popped one request need to be set time at batch allocation time.

Yes, we can also set this popped request time in (2), just before return it to use.

Since all requests in batch allocation use the same alloc and start time, so this patch
just leave it as it is, and reset it at actual used time.

I think both way is ok, do you think it's better to just set the popped one request, leave
other requests time to 0 ? If so, I can update to do it.

Thanks.