From: Christoph Hellwig <hch@infradead.org>
To: Jens Axboe <axboe@kernel.dk>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
keescook@chromium.org,
Linus Torvalds <torvalds@linux-foundation.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] block: switch to atomic_t for request references
Date: Sun, 5 Dec 2021 22:53:49 -0800 [thread overview]
Message-ID: <Ya2zfVAwh4aQ7KVd@infradead.org> (raw)
In-Reply-To: <9f2ad6f1-c1bb-dfac-95c8-7d9eaa7110cc@kernel.dk>
On Fri, Dec 03, 2021 at 08:35:40AM -0700, Jens Axboe wrote:
> refcount_t is not as expensive as it used to be, but it's still more
> expensive than the io_uring method of using atomic_t and just checking
> for potential over/underflow.
>
> This borrows that same implementation, which in turn is based on the
> mm implementation from Linus.
If refcount_t isn't good enough for a normal kernel fast path we have
a problem. Can we discuss that with the maintainers instead of coming
up with our home grown schemes again?
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> ---
>
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index f78bb39e589e..e4df894189ce 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -229,7 +229,7 @@ static void flush_end_io(struct request *flush_rq, blk_status_t error)
> /* release the tag's ownership to the req cloned from */
> spin_lock_irqsave(&fq->mq_flush_lock, flags);
>
> - if (!refcount_dec_and_test(&flush_rq->ref)) {
> + if (!req_ref_put_and_test(flush_rq)) {
> fq->rq_status = error;
> spin_unlock_irqrestore(&fq->mq_flush_lock, flags);
> return;
> @@ -349,7 +349,7 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq,
> * and READ flush_rq->end_io
> */
> smp_wmb();
> - refcount_set(&flush_rq->ref, 1);
> + req_ref_set(flush_rq, 1);
>
> blk_flush_queue_rq(flush_rq, false);
> }
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 995336abee33..380e2dd31bfc 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -228,7 +228,7 @@ static struct request *blk_mq_find_and_get_req(struct blk_mq_tags *tags,
>
> spin_lock_irqsave(&tags->lock, flags);
> rq = tags->rqs[bitnr];
> - if (!rq || rq->tag != bitnr || !refcount_inc_not_zero(&rq->ref))
> + if (!rq || rq->tag != bitnr || !req_ref_inc_not_zero(rq))
> rq = NULL;
> spin_unlock_irqrestore(&tags->lock, flags);
> return rq;
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index fa464a3e2f9a..22ec21aa0c22 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -386,7 +386,7 @@ static struct request *blk_mq_rq_ctx_init(struct blk_mq_alloc_data *data,
> INIT_LIST_HEAD(&rq->queuelist);
> /* tag was already set */
> WRITE_ONCE(rq->deadline, 0);
> - refcount_set(&rq->ref, 1);
> + req_ref_set(rq, 1);
>
> if (rq->rq_flags & RQF_ELV) {
> struct elevator_queue *e = data->q->elevator;
> @@ -634,7 +634,7 @@ void blk_mq_free_request(struct request *rq)
> rq_qos_done(q, rq);
>
> WRITE_ONCE(rq->state, MQ_RQ_IDLE);
> - if (refcount_dec_and_test(&rq->ref))
> + if (req_ref_put_and_test(rq))
> __blk_mq_free_request(rq);
> }
> EXPORT_SYMBOL_GPL(blk_mq_free_request);
> @@ -930,7 +930,7 @@ void blk_mq_end_request_batch(struct io_comp_batch *iob)
> rq_qos_done(rq->q, rq);
>
> WRITE_ONCE(rq->state, MQ_RQ_IDLE);
> - if (!refcount_dec_and_test(&rq->ref))
> + if (!req_ref_put_and_test(rq))
> continue;
>
> blk_crypto_free_request(rq);
> @@ -1373,7 +1373,7 @@ void blk_mq_put_rq_ref(struct request *rq)
> {
> if (is_flush_rq(rq))
> rq->end_io(rq, 0);
> - else if (refcount_dec_and_test(&rq->ref))
> + else if (req_ref_put_and_test(rq))
> __blk_mq_free_request(rq);
> }
>
> @@ -3003,7 +3003,7 @@ static void blk_mq_clear_rq_mapping(struct blk_mq_tags *drv_tags,
> unsigned long rq_addr = (unsigned long)rq;
>
> if (rq_addr >= start && rq_addr < end) {
> - WARN_ON_ONCE(refcount_read(&rq->ref) != 0);
> + WARN_ON_ONCE(req_ref_read(rq) != 0);
> cmpxchg(&drv_tags->rqs[i], rq, NULL);
> }
> }
> @@ -3337,7 +3337,7 @@ static void blk_mq_clear_flush_rq_mapping(struct blk_mq_tags *tags,
> if (!tags)
> return;
>
> - WARN_ON_ONCE(refcount_read(&flush_rq->ref) != 0);
> + WARN_ON_ONCE(req_ref_read(flush_rq) != 0);
>
> for (i = 0; i < queue_depth; i++)
> cmpxchg(&tags->rqs[i], flush_rq, NULL);
> diff --git a/block/blk.h b/block/blk.h
> index 296411900c55..f869f4b2dec9 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -473,4 +473,35 @@ static inline bool should_fail_request(struct block_device *part,
> }
> #endif /* CONFIG_FAIL_MAKE_REQUEST */
>
> +/*
> + * Optimized request reference counting. Ideally we'd make timeouts be more
> + * clever, as that's the only reason we need references at all... But until
> + * this happens, this is faster than using refcount_t. Also see:
> + *
> + * abc54d634334 ("io_uring: switch to atomic_t for io_kiocb reference count")
> + */
> +#define req_ref_zero_or_close_to_overflow(req) \
> + ((unsigned int) atomic_read(&(req->ref)) + 127u <= 127u)
> +
> +static inline bool req_ref_inc_not_zero(struct request *req)
> +{
> + return atomic_inc_not_zero(&req->ref);
> +}
> +
> +static inline bool req_ref_put_and_test(struct request *req)
> +{
> + WARN_ON_ONCE(req_ref_zero_or_close_to_overflow(req));
> + return atomic_dec_and_test(&req->ref);
> +}
> +
> +static inline void req_ref_set(struct request *req, int value)
> +{
> + atomic_set(&req->ref, value);
> +}
> +
> +static inline int req_ref_read(struct request *req)
> +{
> + return atomic_read(&req->ref);
> +}
> +
> #endif /* BLK_INTERNAL_H */
> diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
> index bfc3cc61f653..ecdc049b52fa 100644
> --- a/include/linux/blk-mq.h
> +++ b/include/linux/blk-mq.h
> @@ -138,7 +138,7 @@ struct request {
> unsigned short ioprio;
>
> enum mq_rq_state state;
> - refcount_t ref;
> + atomic_t ref;
>
> unsigned long deadline;
>
> --
> Jens Axboe
>
---end quoted text---
next parent reply other threads:[~2021-12-06 6:53 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <9f2ad6f1-c1bb-dfac-95c8-7d9eaa7110cc@kernel.dk>
2021-12-06 6:53 ` Christoph Hellwig [this message]
2021-12-06 8:31 ` [PATCH] block: switch to atomic_t for request references Peter Zijlstra
2021-12-06 16:32 ` Jens Axboe
2021-12-06 17:19 ` Peter Zijlstra
2021-12-06 17:35 ` Linus Torvalds
2021-12-06 18:13 ` Jens Axboe
2021-12-06 20:51 ` Kees Cook
2021-12-06 21:17 ` Linus Torvalds
2021-12-06 23:28 ` Kees Cook
2021-12-07 0:13 ` Linus Torvalds
2021-12-07 4:56 ` Kees Cook
2021-12-07 9:34 ` Peter Zijlstra
2021-12-07 16:03 ` Linus Torvalds
2021-12-07 10:30 ` Peter Zijlstra
2021-12-07 16:10 ` Linus Torvalds
2021-12-07 16:23 ` Peter Zijlstra
2021-12-06 16:31 ` Jens Axboe
2021-12-07 11:26 ` Peter Zijlstra
2021-12-07 13:28 ` Peter Zijlstra
2021-12-07 15:51 ` Peter Zijlstra
2021-12-07 16:13 ` Linus Torvalds
2021-12-07 16:52 ` Peter Zijlstra
2021-12-07 17:41 ` Peter Zijlstra
2021-12-07 17:43 ` Linus Torvalds
2021-12-07 17:45 ` Linus Torvalds
2021-12-07 20:28 ` Peter Zijlstra
2021-12-07 23:23 ` Linus Torvalds
2021-12-08 17:07 ` Peter Zijlstra
2021-12-08 18:00 ` Linus Torvalds
2021-12-08 18:44 ` Peter Zijlstra
2021-12-08 18:50 ` Linus Torvalds
2021-12-08 20:32 ` Peter Zijlstra
2021-12-10 10:57 ` Peter Zijlstra
2021-12-10 12:38 ` Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ya2zfVAwh4aQ7KVd@infradead.org \
--to=hch@infradead.org \
--cc=axboe@kernel.dk \
--cc=keescook@chromium.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).