[PATCH RT] blk-mq: revert raw locks, post pone notifier to POST_DEAD

[PATCH RT] blk-mq: revert raw locks, post pone notifier to POST_DEAD

[Sebastian Andrzej Siewior]

The blk_mq_cpu_notify_lock should be raw because some CPU down levels
are called with interrupts off. The notifier itself calls currently one
function that is blk_mq_hctx_notify().
That function acquires the ctx->lock lock which is sleeping and I would
prefer to keep it that way. That function only moves IO-requests from
the CPU that is going offline to another CPU and it is currently the
only one. Therefore I revert the list lock back to sleeping spinlocks
and let the notifier run at POST_DEAD time.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Mike, I see that lockdep splat (sleeping while atomic) during cpu-hotplug.
Don't you see this, too?

 block/blk-mq-cpu.c | 17 ++++++++++-------
 block/blk-mq.c     |  2 +-
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/block/blk-mq-cpu.c b/block/blk-mq-cpu.c
index 136ef86..37acc3a 100644
--- a/block/blk-mq-cpu.c
+++ b/block/blk-mq-cpu.c
@@ -11,7 +11,7 @@
 #include "blk-mq.h"
 
 static LIST_HEAD(blk_mq_cpu_notify_list);
-static DEFINE_RAW_SPINLOCK(blk_mq_cpu_notify_lock);
+static DEFINE_SPINLOCK(blk_mq_cpu_notify_lock);
 
 static int blk_mq_main_cpu_notify(struct notifier_block *self,
 				  unsigned long action, void *hcpu)
@@ -19,12 +19,15 @@ static int blk_mq_main_cpu_notify(struct notifier_block *self,
 	unsigned int cpu = (unsigned long) hcpu;
 	struct blk_mq_cpu_notifier *notify;
 
-	raw_spin_lock(&blk_mq_cpu_notify_lock);
+	if (action != CPU_POST_DEAD && action != CPU_POST_DEAD)
+		return NOTIFY_OK;
+
+	spin_lock(&blk_mq_cpu_notify_lock);
 
 	list_for_each_entry(notify, &blk_mq_cpu_notify_list, list)
 		notify->notify(notify->data, action, cpu);
 
-	raw_spin_unlock(&blk_mq_cpu_notify_lock);
+	spin_unlock(&blk_mq_cpu_notify_lock);
 	return NOTIFY_OK;
 }
 
@@ -32,16 +35,16 @@ void blk_mq_register_cpu_notifier(struct blk_mq_cpu_notifier *notifier)
 {
 	BUG_ON(!notifier->notify);
 
-	raw_spin_lock(&blk_mq_cpu_notify_lock);
+	spin_lock(&blk_mq_cpu_notify_lock);
 	list_add_tail(&notifier->list, &blk_mq_cpu_notify_list);
-	raw_spin_unlock(&blk_mq_cpu_notify_lock);
+	spin_unlock(&blk_mq_cpu_notify_lock);
 }
 
 void blk_mq_unregister_cpu_notifier(struct blk_mq_cpu_notifier *notifier)
 {
-	raw_spin_lock(&blk_mq_cpu_notify_lock);
+	spin_lock(&blk_mq_cpu_notify_lock);
 	list_del(&notifier->list);
-	raw_spin_unlock(&blk_mq_cpu_notify_lock);
+	spin_unlock(&blk_mq_cpu_notify_lock);
 }
 
 void blk_mq_init_cpu_notifier(struct blk_mq_cpu_notifier *notifier,
diff --git a/block/blk-mq.c b/block/blk-mq.c
index da3af9f..d5e73d8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -971,7 +971,7 @@ static void blk_mq_hctx_notify(void *data, unsigned long action,
 	struct blk_mq_ctx *ctx;
 	LIST_HEAD(tmp);
 
-	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
+	if (action != CPU_POST_DEAD && action != CPU_POST_DEAD)
 		return;
 
 	/*
-- 
2.0.0.rc0

Re: [PATCH RT] blk-mq: revert raw locks, post pone notifier to POST_DEAD

[Mike Galbraith]

On Sat, 2014-05-03 at 18:42 +0200, Sebastian Andrzej Siewior wrote: 
> The blk_mq_cpu_notify_lock should be raw because some CPU down levels
> are called with interrupts off. The notifier itself calls currently one
> function that is blk_mq_hctx_notify().
> That function acquires the ctx->lock lock which is sleeping and I would
> prefer to keep it that way. That function only moves IO-requests from
> the CPU that is going offline to another CPU and it is currently the
> only one. Therefore I revert the list lock back to sleeping spinlocks
> and let the notifier run at POST_DEAD time.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Mike, I see that lockdep splat (sleeping while atomic) during cpu-hotplug.
> Don't you see this, too?

Nope, didn't.

> block/blk-mq-cpu.c | 17 ++++++++++-------
>  block/blk-mq.c     |  2 +-
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/block/blk-mq-cpu.c b/block/blk-mq-cpu.c
> index 136ef86..37acc3a 100644
> --- a/block/blk-mq-cpu.c
> +++ b/block/blk-mq-cpu.c
> @@ -11,7 +11,7 @@
>  #include "blk-mq.h"
>  
>  static LIST_HEAD(blk_mq_cpu_notify_list);
> -static DEFINE_RAW_SPINLOCK(blk_mq_cpu_notify_lock);
> +static DEFINE_SPINLOCK(blk_mq_cpu_notify_lock);
>  
>  static int blk_mq_main_cpu_notify(struct notifier_block *self,
>  				  unsigned long action, void *hcpu)
> @@ -19,12 +19,15 @@ static int blk_mq_main_cpu_notify(struct notifier_block *self,
>  	unsigned int cpu = (unsigned long) hcpu;
>  	struct blk_mq_cpu_notifier *notify;
>  
> -	raw_spin_lock(&blk_mq_cpu_notify_lock);
> +	if (action != CPU_POST_DEAD && action != CPU_POST_DEAD)
> +		return NOTIFY_OK;
> +
> +	spin_lock(&blk_mq_cpu_notify_lock);
>  
>  	list_for_each_entry(notify, &blk_mq_cpu_notify_list, list)
>  		notify->notify(notify->data, action, cpu);
>  
> -	raw_spin_unlock(&blk_mq_cpu_notify_lock);
> +	spin_unlock(&blk_mq_cpu_notify_lock);
>  	return NOTIFY_OK;
>  }
>  
> @@ -32,16 +35,16 @@ void blk_mq_register_cpu_notifier(struct blk_mq_cpu_notifier *notifier)
>  {
>  	BUG_ON(!notifier->notify);
>  
> -	raw_spin_lock(&blk_mq_cpu_notify_lock);
> +	spin_lock(&blk_mq_cpu_notify_lock);
>  	list_add_tail(&notifier->list, &blk_mq_cpu_notify_list);
> -	raw_spin_unlock(&blk_mq_cpu_notify_lock);
> +	spin_unlock(&blk_mq_cpu_notify_lock);
>  }
>  
>  void blk_mq_unregister_cpu_notifier(struct blk_mq_cpu_notifier *notifier)
>  {
> -	raw_spin_lock(&blk_mq_cpu_notify_lock);
> +	spin_lock(&blk_mq_cpu_notify_lock);
>  	list_del(&notifier->list);
> -	raw_spin_unlock(&blk_mq_cpu_notify_lock);
> +	spin_unlock(&blk_mq_cpu_notify_lock);
>  }
>  
>  void blk_mq_init_cpu_notifier(struct blk_mq_cpu_notifier *notifier,
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index da3af9f..d5e73d8 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -971,7 +971,7 @@ static void blk_mq_hctx_notify(void *data, unsigned long action,
>  	struct blk_mq_ctx *ctx;
>  	LIST_HEAD(tmp);
>  
> -	if (action != CPU_DEAD && action != CPU_DEAD_FROZEN)
> +	if (action != CPU_POST_DEAD && action != CPU_POST_DEAD)
>  		return;
>  
>  	/*

Re: [PATCH RT] blk-mq: revert raw locks, post pone notifier to POST_DEAD

[Sebastian Andrzej Siewior]

On 05/03/2014 07:03 PM, Mike Galbraith wrote:
>> Mike, I see that lockdep splat (sleeping while atomic) during cpu-hotplug.
>> Don't you see this, too?
> 
> Nope, didn't.

Hmm. blk_mq_hctx_notify(). It does
	spin_lock(&ctx->lock);

Isn't this a sleeping spinlock for you or don't get here at all?

Sebastian

Re: [PATCH RT] blk-mq: revert raw locks, post pone notifier to POST_DEAD

[Mike Galbraith]

On Wed, 2014-05-07 at 10:19 +0200, Sebastian Andrzej Siewior wrote: 
> On 05/03/2014 07:03 PM, Mike Galbraith wrote:
> >> Mike, I see that lockdep splat (sleeping while atomic) during cpu-hotplug.
> >> Don't you see this, too?
> > 
> > Nope, didn't.
> 
> Hmm. blk_mq_hctx_notify(). It does
> 	spin_lock(&ctx->lock);
> 
> Isn't this a sleeping spinlock for you or don't get here at all?

Hm, cts->lock...

git@marge:~/suse/scratch_area/linux-3.12-SLE12-RT> grep -IR cpu_lock block
git@marge:~/suse/scratch_area/linux-3.12-SLE12-RT>

(here lock lock lock, here boy)

marge:/usr/local/src/kernel/linux-3.x.git # grep -IR cpu_lock block
marge:/usr/local/src/kernel/linux-3.x.git # 

marge:/usr/local/src/kernel/linux-3.14 # grep -IR cpu_lock block
marge:/usr/local/src/kernel/linux-3.14 #

(what the..)

marge:/usr/local/src/kernel/linux-3.14-rt # grep -IR cpu_lock block
block/blk-mq.c: spin_lock(&ctx->cpu_lock);
block/blk-mq.c: spin_unlock(&ctx->cpu_lock);
block/blk-mq.c:                         spin_unlock(&ctx->cpu_lock);
block/blk-mq.c:                         spin_lock(&ctx->cpu_lock);
block/blk-mq.c:         spin_lock_init(&__ctx->cpu_lock);
block/blk-mq.h: spinlock_t              cpu_lock;
marge:/usr/local/src/kernel/linux-3.14-rt #

(aha!)

Nope, I definitely don't get there.

	-Mike