Skip to contentSkip to conversation

block: sbitmap tweaks

PATCH In review Omar Sandoval 3 participants 5 messages Feb 28, 2018

Subsystems (2)
Block layer Library code
lore
Share & tools
lore

Omar Sandoval

author
#0 lore raw

cover block: sbitmap tweaks

From: Omar Sandoval <[email protected]>

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(-)
Signature · 1 line
2.16.2
author
#1 lore raw

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

From: Omar Sandoval <[email protected]>

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 <[email protected]>
---
 block/blk-mq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
block/blk-mq.c +1 -1
index 357492712b0e..09a57d97ca7f 100644
@@ -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;
 }
Signature · 1 line
2.16.2
author
#2 lore raw

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

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

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 <[email protected]>
Signed-off-by Omar Sandoval <[email protected]>
---
 include/linux/sbitmap.h |  8 ++++++++
 lib/sbitmap.c           | 10 +++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)
include/linux/sbitmap.h +8 -0
index 0dcc60e820de..841585f6e5f2 100644
@@ -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));
lib/sbitmap.c +5 -5
index 42b5ca0acf93..e6a9c06ec70c 100644
@@ -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;
Signature · 1 line
2.16.2
Acked-by on the v1 series
#4 lore raw
In reply to Omar Sandoval
Quoted 9 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

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 <[email protected]>

Thanks.

Signature · 1 line
tejun