Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Thu, 13 Jul 2023 07:58:24 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Chengming Zhou <chengming.zhou@linux.dev>
Cc:     Jens Axboe <axboe@kernel.dk>, hch@lst.de,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        ming.lei@redhat.com, zhouchengming@bytedance.com
Subject: Re: [PATCH v5] blk-mq: fix start_time_ns and alloc_time_ns for
 pre-allocated rq
Message-ID: <ZLA7QAfSojxu_FMW@slm.duckdns.org>
References: <20230710105516.2053478-1-chengming.zhou@linux.dev>
 <aa813164-9a6a-53bd-405b-ba4cc1f1b656@kernel.dk>
 <63f93f1c-98da-4c09-b3d8-711f6953d8b7@linux.dev>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <63f93f1c-98da-4c09-b3d8-711f6953d8b7@linux.dev>
Precedence: bulk

Hello,

On Thu, Jul 13, 2023 at 08:25:50PM +0800, Chengming Zhou wrote:
> Ok, this version will only get time stamp once for one request, it's actually
> not worse than the current code, which will get start time stamp once for each
> request even in the batch allocation.
> 
> But yes, maybe we can also set the start time stamp in the batch mode, and only
> update the time stamp in the block case, like you said, has better performance.
> 
> The first version [1] I posted actually just did this, in which use a nr_flush counter
> in plug to indicate that we blocked & flushed plug. Tejun and I think it seems fragile.
> So go to this way that only set time stamp once when the request actually used.
> 
> [1] https://lore.kernel.org/all/20230601053919.3639954-1-chengming.zhou@linux.dev/
> 
> Another way I can think of is to make rq_qos_throttle() return a bool to indicate
> if it blocked. Tejun and Jens, how do you think about this way?
> 
> Although it's better performance, in case of preemption, the time stamp maybe not accurate.

Trying to manually optimized timestamp reads seems like a bit of fool's
errand to me. I don't think anyone cares about nanosec accuracy, so there
are ample opportunities for generically caching timestamp so that we don't
have to contort code to optimzie timestamp calls.

It's a bit out of scope for this patchset but I think it might make sense to
build a timestamp caching infrastructure. The cached timestamp can be
invalidated on context switches (block layer already hooks into them) and
issue and other path boundaries (e.g. at the end of plug flush).

Thanks.

-- 
tejun