[PATCH 0/2] block: sbitmap tweaks

[PATCH 0/2] block: sbitmap tweaks

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

Two fixlets inspired by Tejun's patch
(https://patchwork.kernel.org/patch/10226749/). Patch 2 is what we
discussed on that patch, patch 1 is a small preparation.

Omar Sandoval (2):
  block: clear ctx pending bit under ctx lock
  sbitmap: use test_and_set_bit_lock()/clear_bit_unlock()

 block/blk-mq.c          |  2 +-
 include/linux/sbitmap.h |  8 ++++++++
 lib/sbitmap.c           | 10 +++++-----
 3 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.16.2

[PATCH 1/2] block: clear ctx pending bit under ctx lock

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

When we insert a request, we set the software queue pending bit while
holding the software queue lock. However, we clear it outside of the
lock, so it's possible that a concurrent insert could reset the bit
after we clear it but before we empty the request list. Afterwards, the
bit would still be set but the software queue wouldn't have any requests
in it, leading us to do a spurious run in the future. This is mostly a
benign/theoretical issue, but it makes the following change easier to
justify.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 357492712b0e..09a57d97ca7f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -984,9 +984,9 @@ static bool flush_busy_ctx(struct sbitmap *sb, unsigned int bitnr, void *data)
 	struct blk_mq_hw_ctx *hctx = flush_data->hctx;
 	struct blk_mq_ctx *ctx = hctx->ctxs[bitnr];
 
-	sbitmap_clear_bit(sb, bitnr);
 	spin_lock(&ctx->lock);
 	list_splice_tail_init(&ctx->rq_list, flush_data->list);
+	sbitmap_clear_bit(sb, bitnr);
 	spin_unlock(&ctx->lock);
 	return true;
 }
-- 
2.16.2

[PATCH 2/2] sbitmap: use test_and_set_bit_lock()/clear_bit_unlock()

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

sbitmap_queue_get()/sbitmap_queue_clear() are used for
allocating/freeing a resource, so they should provide acquire/release
barrier semantics, respectively. sbitmap_get() currently contains a full
barrier, which is unnecessary, so use test_and_set_bit_lock() instead of
test_and_set_bit() (these are equivalent on x86_64). sbitmap_clear_bit()
does not imply any barriers, which is incorrect, as accesses of the
resource (e.g., request) could potentially get reordered to after the
clear_bit(). Introduce sbitmap_clear_bit_unlock() and use it for
sbitmap_queue_clear() (this only adds a compiler barrier on x86_64). The
other existing user of sbitmap_clear_bit() (the blk-mq software queue
pending map) is serialized through a spinlock and does not need this.

Reported-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 include/linux/sbitmap.h |  8 ++++++++
 lib/sbitmap.c           | 10 +++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/linux/sbitmap.h b/include/linux/sbitmap.h
index 0dcc60e820de..841585f6e5f2 100644
--- a/include/linux/sbitmap.h
+++ b/include/linux/sbitmap.h
@@ -171,6 +171,8 @@ void sbitmap_resize(struct sbitmap *sb, unsigned int depth);
  *               starting from the last allocated bit. This is less efficient
  *               than the default behavior (false).
  *
+ * This operation provides acquire barrier semantics if it succeeds.
+ *
  * Return: Non-negative allocated bit number if successful, -1 otherwise.
  */
 int sbitmap_get(struct sbitmap *sb, unsigned int alloc_hint, bool round_robin);
@@ -300,6 +302,12 @@ static inline void sbitmap_clear_bit(struct sbitmap *sb, unsigned int bitnr)
 	clear_bit(SB_NR_TO_BIT(sb, bitnr), __sbitmap_word(sb, bitnr));
 }
 
+static inline void sbitmap_clear_bit_unlock(struct sbitmap *sb,
+					    unsigned int bitnr)
+{
+	clear_bit_unlock(SB_NR_TO_BIT(sb, bitnr), __sbitmap_word(sb, bitnr));
+}
+
 static inline int sbitmap_test_bit(struct sbitmap *sb, unsigned int bitnr)
 {
 	return test_bit(SB_NR_TO_BIT(sb, bitnr), __sbitmap_word(sb, bitnr));
diff --git a/lib/sbitmap.c b/lib/sbitmap.c
index 42b5ca0acf93..e6a9c06ec70c 100644
--- a/lib/sbitmap.c
+++ b/lib/sbitmap.c
@@ -100,7 +100,7 @@ static int __sbitmap_get_word(unsigned long *word, unsigned long depth,
 			return -1;
 		}
 
-		if (!test_and_set_bit(nr, word))
+		if (!test_and_set_bit_lock(nr, word))
 			break;
 
 		hint = nr + 1;
@@ -434,9 +434,9 @@ static void sbq_wake_up(struct sbitmap_queue *sbq)
 	/*
 	 * Pairs with the memory barrier in set_current_state() to ensure the
 	 * proper ordering of clear_bit()/waitqueue_active() in the waker and
-	 * test_and_set_bit()/prepare_to_wait()/finish_wait() in the waiter. See
-	 * the comment on waitqueue_active(). This is __after_atomic because we
-	 * just did clear_bit() in the caller.
+	 * test_and_set_bit_lock()/prepare_to_wait()/finish_wait() in the
+	 * waiter. See the comment on waitqueue_active(). This is __after_atomic
+	 * because we just did clear_bit_unlock() in the caller.
 	 */
 	smp_mb__after_atomic();
 
@@ -469,7 +469,7 @@ static void sbq_wake_up(struct sbitmap_queue *sbq)
 void sbitmap_queue_clear(struct sbitmap_queue *sbq, unsigned int nr,
 			 unsigned int cpu)
 {
-	sbitmap_clear_bit(&sbq->sb, nr);
+	sbitmap_clear_bit_unlock(&sbq->sb, nr);
 	sbq_wake_up(sbq);
 	if (likely(!sbq->round_robin && nr < sbq->sb.depth))
 		*per_cpu_ptr(sbq->alloc_hint, cpu) = nr;
-- 
2.16.2

Re: [PATCH 0/2] block: sbitmap tweaks

[Jens Axboe]

On 2/27/18 5:56 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Two fixlets inspired by Tejun's patch
> (https://patchwork.kernel.org/patch/10226749/). Patch 2 is what we
> discussed on that patch, patch 1 is a small preparation.

Unless I hear complaints, I'm going to queue these up for 4.17
later today.

-- 
Jens Axboe

Re: [PATCH 0/2] block: sbitmap tweaks

[Tejun Heo]

On Tue, Feb 27, 2018 at 04:56:41PM -0800, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> Two fixlets inspired by Tejun's patch
> (https://patchwork.kernel.org/patch/10226749/). Patch 2 is what we
> discussed on that patch, patch 1 is a small preparation.
> 
> Omar Sandoval (2):
>   block: clear ctx pending bit under ctx lock
>   sbitmap: use test_and_set_bit_lock()/clear_bit_unlock()

FWIW,

 Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun