linux-kernel - Re: [PATCH 3/3] block: avoid hctx spinlock for plug with multiple queues

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] [day] [month] [year] [list]

Message-ID: <CADUfDZpDg5hXeShhd9GX70uVbqv7RU+u-grf7S8j2qdgFXDxYw@mail.gmail.com>
Date: Thu, 24 Apr 2025 18:00:16 -0700
From: Caleb Sander Mateos <csander@...estorage.com>
To: Christoph Hellwig <hch@...radead.org>
Cc: Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] block: avoid hctx spinlock for plug with multiple queues

On Thu, Apr 24, 2025 at 4:38 AM Christoph Hellwig <hch@...radead.org> wrote:
>
> > +static void blk_mq_extract_queue_requests(struct rq_list *rqs,
> > +                                       struct rq_list *queue_rqs,
> > +                                       unsigned *queue_depth)
> > +{
> > +     struct rq_list matched_rqs = {}, unmatched_rqs = {};
> > +     struct request *rq = rq_list_pop(rqs);
> > +     struct request_queue *this_q = rq->q;
> > +     unsigned depth = 1;
> > +
> > +     rq_list_add_tail(&matched_rqs, rq);
> > +     while ((rq = rq_list_pop(rqs))) {
> > +             if (rq->q == this_q) {
> > +                     rq_list_add_tail(&matched_rqs, rq);
> > +                     depth++;
> > +             } else {
> > +                     rq_list_add_tail(&unmatched_rqs, rq);
> > +             }
>
> This might be moe efficient if you keep an extra iterator and never
> mode the request for another queue.

Sure, will do.

>
> > +     }
> > +
> > +     *queue_rqs = matched_rqs;
> > +     *rqs = unmatched_rqs;
> > +     *queue_depth = depth;
>
> .. and I'd return the queue depth here instead of making it another
> output argument.

Okay.

>
> > +static void blk_mq_dispatch_multiple_queue_requests(struct rq_list *rqs)
> > +{
> > +     do {
> > +             struct rq_list queue_rqs;
> > +             unsigned depth;
> > +
> > +             blk_mq_extract_queue_requests(rqs, &queue_rqs, &depth);
> > +             blk_mq_dispatch_queue_requests(&queue_rqs, depth);
> > +             while (!rq_list_empty(&queue_rqs)) {
> > +                     blk_mq_dispatch_list(&queue_rqs, false);
> > +             }
>
> No need for the braces in the inner while loop here.

Old habits die hard :)

>
> The other caller of blk_mq_dispatch_list loops until the list is empty,
> why don't we need that here?

This loop does continue calling blk_mq_dispatch_list() until queue_rqs
is empty. And the outer loop keeps repopulating queue_rqs until the
entire rqs list is empty. Am I misunderstanding you?

Thanks for the review,
Caleb