[PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Xu]

Previously we will raise an warning if we want to insert a csd object
which is with the LOCK flag set, and if it happens we'll also wait for
the lock to be released.  However, this operation does not match
perfectly with how the function is named - the name with "_async"
suffix hints that this function should not block, while we will.

This patch changed this behavior by simply return -EBUSY instead of
waiting, at the meantime we allow this operation to happen without
warning the user to change this into a feature when the caller wants
to "insert a csd object, if it's there, just wait for that one".

This is pretty safe because in flush_smp_call_function_queue() for
async csd objects (where csd->flags&SYNC is zero) we'll first do the
unlock then we call the csd->func().  So if we see the csd->flags&LOCK
is true in smp_call_function_single_async(), then it's guaranteed that
csd->func() will be called after this smp_call_function_single_async()
returns -EBUSY.

Update the comment of the function too to refect this.

CC: Marcelo Tosatti <mtosatti@redhat.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Nadav Amit <namit@vmware.com>
CC: Josh Poimboeuf <jpoimboe@redhat.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: Peter Zijlstra <peterz@infradead.org>
CC: linux-kernel@vger.kernel.org
Signed-off-by: Peter Xu <peterx@redhat.com>
---

The story starts from a test where we've encountered the WARN_ON() on
a customized kernel and the csd_wait() took merely forever to
complete (so we've got a WARN_ON plusing a hang host).  The current
solution (which is downstream-only for now) is that from the caller's
side we use a boolean to store whether the csd is executed, we do:

  if (atomic_cmpxchg(&in_progress, 0, 1))
    smp_call_function_single_async(..);

While at the end of csd->func() we clear the bit.  However imho that's
mostly what csd->flags&LOCK is doing.  So I'm thinking maybe it would
worth it to raise this patch for upstream too so that it might help
other users of smp_call_function_single_async() when they need the
same semantic (and, I do think we shouldn't wait in _async()s...)

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 kernel/smp.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 7dbcb402c2fc..dd31e8228218 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -329,6 +329,11 @@ EXPORT_SYMBOL(smp_call_function_single);
  * (ie: embedded in an object) and is responsible for synchronizing it
  * such that the IPIs performed on the @csd are strictly serialized.
  *
+ * If the function is called with one csd which has not yet been
+ * processed by previous call to smp_call_function_single_async(), the
+ * function will return immediately with -EBUSY showing that the csd
+ * object is still in progress.
+ *
  * NOTE: Be careful, there is unfortunately no current debugging facility to
  * validate the correctness of this serialization.
  */
@@ -338,14 +343,17 @@ int smp_call_function_single_async(int cpu, call_single_data_t *csd)
 
 	preempt_disable();
 
-	/* We could deadlock if we have to wait here with interrupts disabled! */
-	if (WARN_ON_ONCE(csd->flags & CSD_FLAG_LOCK))
-		csd_lock_wait(csd);
+	if (csd->flags & CSD_FLAG_LOCK) {
+		err = -EBUSY;
+		goto out;
+	}
 
 	csd->flags = CSD_FLAG_LOCK;
 	smp_wmb();
 
 	err = generic_exec_single(cpu, csd, csd->func, csd->info);
+
+out:
 	preempt_enable();
 
 	return err;
-- 
2.21.0

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Zijlstra]

On Wed, Dec 04, 2019 at 03:48:23PM -0500, Peter Xu wrote:
> Previously we will raise an warning if we want to insert a csd object
> which is with the LOCK flag set, and if it happens we'll also wait for
> the lock to be released.  However, this operation does not match
> perfectly with how the function is named - the name with "_async"
> suffix hints that this function should not block, while we will.
> 
> This patch changed this behavior by simply return -EBUSY instead of
> waiting, at the meantime we allow this operation to happen without
> warning the user to change this into a feature when the caller wants
> to "insert a csd object, if it's there, just wait for that one".
> 
> This is pretty safe because in flush_smp_call_function_queue() for
> async csd objects (where csd->flags&SYNC is zero) we'll first do the
> unlock then we call the csd->func().  So if we see the csd->flags&LOCK
> is true in smp_call_function_single_async(), then it's guaranteed that
> csd->func() will be called after this smp_call_function_single_async()
> returns -EBUSY.
> 
> Update the comment of the function too to refect this.
> 
> CC: Marcelo Tosatti <mtosatti@redhat.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Nadav Amit <namit@vmware.com>
> CC: Josh Poimboeuf <jpoimboe@redhat.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> The story starts from a test where we've encountered the WARN_ON() on
> a customized kernel and the csd_wait() took merely forever to
> complete (so we've got a WARN_ON plusing a hang host).  The current
> solution (which is downstream-only for now) is that from the caller's
> side we use a boolean to store whether the csd is executed, we do:
> 
>   if (atomic_cmpxchg(&in_progress, 0, 1))
>     smp_call_function_single_async(..);
> 
> While at the end of csd->func() we clear the bit.  However imho that's
> mostly what csd->flags&LOCK is doing.  So I'm thinking maybe it would
> worth it to raise this patch for upstream too so that it might help
> other users of smp_call_function_single_async() when they need the
> same semantic (and, I do think we shouldn't wait in _async()s...)

hrtick_start() seems to employ something similar.

> Signed-off-by: Peter Xu <peterx@redhat.com>

duplicate tag :-)

> ---
>  kernel/smp.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 7dbcb402c2fc..dd31e8228218 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -329,6 +329,11 @@ EXPORT_SYMBOL(smp_call_function_single);
>   * (ie: embedded in an object) and is responsible for synchronizing it
>   * such that the IPIs performed on the @csd are strictly serialized.
>   *
> + * If the function is called with one csd which has not yet been
> + * processed by previous call to smp_call_function_single_async(), the
> + * function will return immediately with -EBUSY showing that the csd
> + * object is still in progress.
> + *
>   * NOTE: Be careful, there is unfortunately no current debugging facility to
>   * validate the correctness of this serialization.
>   */
> @@ -338,14 +343,17 @@ int smp_call_function_single_async(int cpu, call_single_data_t *csd)
>  
>  	preempt_disable();
>  
> -	/* We could deadlock if we have to wait here with interrupts disabled! */
> -	if (WARN_ON_ONCE(csd->flags & CSD_FLAG_LOCK))
> -		csd_lock_wait(csd);
> +	if (csd->flags & CSD_FLAG_LOCK) {
> +		err = -EBUSY;
> +		goto out;
> +	}
>  
>  	csd->flags = CSD_FLAG_LOCK;
>  	smp_wmb();
>  
>  	err = generic_exec_single(cpu, csd, csd->func, csd->info);
> +
> +out:
>  	preempt_enable();
>  
>  	return err;

Yes.. I think this will work.

I worry though; usage such as in __blk_mq_complete_request() /
raise_blk_irq(), which explicitly clears csd.flags before calling it
seems more dangerous than usual.

liquidio_napi_drv_callback() does that same.

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Xu]

On Wed, Dec 11, 2019 at 04:40:58PM +0100, Peter Zijlstra wrote:
> On Wed, Dec 04, 2019 at 03:48:23PM -0500, Peter Xu wrote:
> > Previously we will raise an warning if we want to insert a csd object
> > which is with the LOCK flag set, and if it happens we'll also wait for
> > the lock to be released.  However, this operation does not match
> > perfectly with how the function is named - the name with "_async"
> > suffix hints that this function should not block, while we will.
> > 
> > This patch changed this behavior by simply return -EBUSY instead of
> > waiting, at the meantime we allow this operation to happen without
> > warning the user to change this into a feature when the caller wants
> > to "insert a csd object, if it's there, just wait for that one".
> > 
> > This is pretty safe because in flush_smp_call_function_queue() for
> > async csd objects (where csd->flags&SYNC is zero) we'll first do the
> > unlock then we call the csd->func().  So if we see the csd->flags&LOCK
> > is true in smp_call_function_single_async(), then it's guaranteed that
> > csd->func() will be called after this smp_call_function_single_async()
> > returns -EBUSY.
> > 
> > Update the comment of the function too to refect this.
> > 
> > CC: Marcelo Tosatti <mtosatti@redhat.com>
> > CC: Thomas Gleixner <tglx@linutronix.de>
> > CC: Nadav Amit <namit@vmware.com>
> > CC: Josh Poimboeuf <jpoimboe@redhat.com>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > CC: Peter Zijlstra <peterz@infradead.org>
> > CC: linux-kernel@vger.kernel.org
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 
> > The story starts from a test where we've encountered the WARN_ON() on
> > a customized kernel and the csd_wait() took merely forever to
> > complete (so we've got a WARN_ON plusing a hang host).  The current
> > solution (which is downstream-only for now) is that from the caller's
> > side we use a boolean to store whether the csd is executed, we do:
> > 
> >   if (atomic_cmpxchg(&in_progress, 0, 1))
> >     smp_call_function_single_async(..);
> > 
> > While at the end of csd->func() we clear the bit.  However imho that's
> > mostly what csd->flags&LOCK is doing.  So I'm thinking maybe it would
> > worth it to raise this patch for upstream too so that it might help
> > other users of smp_call_function_single_async() when they need the
> > same semantic (and, I do think we shouldn't wait in _async()s...)
> 
> hrtick_start() seems to employ something similar.

True.  More "statistics" below.

> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> duplicate tag :-)

Oops, I'll remove that when I repost (of course, if at least any of
you would still like me to repost :).

> 
> > ---
> >  kernel/smp.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/smp.c b/kernel/smp.c
> > index 7dbcb402c2fc..dd31e8228218 100644
> > --- a/kernel/smp.c
> > +++ b/kernel/smp.c
> > @@ -329,6 +329,11 @@ EXPORT_SYMBOL(smp_call_function_single);
> >   * (ie: embedded in an object) and is responsible for synchronizing it
> >   * such that the IPIs performed on the @csd are strictly serialized.
> >   *
> > + * If the function is called with one csd which has not yet been
> > + * processed by previous call to smp_call_function_single_async(), the
> > + * function will return immediately with -EBUSY showing that the csd
> > + * object is still in progress.
> > + *
> >   * NOTE: Be careful, there is unfortunately no current debugging facility to
> >   * validate the correctness of this serialization.
> >   */
> > @@ -338,14 +343,17 @@ int smp_call_function_single_async(int cpu, call_single_data_t *csd)
> >  
> >  	preempt_disable();
> >  
> > -	/* We could deadlock if we have to wait here with interrupts disabled! */
> > -	if (WARN_ON_ONCE(csd->flags & CSD_FLAG_LOCK))
> > -		csd_lock_wait(csd);
> > +	if (csd->flags & CSD_FLAG_LOCK) {
> > +		err = -EBUSY;
> > +		goto out;
> > +	}
> >  
> >  	csd->flags = CSD_FLAG_LOCK;
> >  	smp_wmb();
> >  
> >  	err = generic_exec_single(cpu, csd, csd->func, csd->info);
> > +
> > +out:
> >  	preempt_enable();
> >  
> >  	return err;
> 
> Yes.. I think this will work.
> 
> I worry though; usage such as in __blk_mq_complete_request() /
> raise_blk_irq(), which explicitly clears csd.flags before calling it
> seems more dangerous than usual.
> 
> liquidio_napi_drv_callback() does that same.

This is also true.

Here's the statistics I mentioned:

=================================================

(1) Implemented the same counter mechanism on the caller's:

*** arch/mips/kernel/smp.c:
tick_broadcast[713]            smp_call_function_single_async(cpu, csd);
*** drivers/cpuidle/coupled.c:
cpuidle_coupled_poke[336]      smp_call_function_single_async(cpu, csd);
*** kernel/sched/core.c:
hrtick_start[298]              smp_call_function_single_async(cpu_of(rq), &rq->hrtick_csd);

(2) Cleared the csd flags before calls:

*** arch/s390/pci/pci_irq.c:
zpci_handle_fallback_irq[185]  smp_call_function_single_async(cpu, &cpu_data->csd);
*** block/blk-mq.c:
__blk_mq_complete_request[622] smp_call_function_single_async(ctx->cpu, &rq->csd);
*** block/blk-softirq.c:
raise_blk_irq[70]              smp_call_function_single_async(cpu, data);
*** drivers/net/ethernet/cavium/liquidio/lio_core.c:
liquidio_napi_drv_callback[735] smp_call_function_single_async(droq->cpu_id, csd);

(3) Others:

*** arch/mips/kernel/process.c:
raise_backtrace[713]           smp_call_function_single_async(cpu, csd);
*** arch/x86/kernel/cpuid.c:
cpuid_read[85]                 err = smp_call_function_single_async(cpu, &csd);
*** arch/x86/lib/msr-smp.c:
rdmsr_safe_on_cpu[182]         err = smp_call_function_single_async(cpu, &csd);
*** include/linux/smp.h:
bool[60]                       int smp_call_function_single_async(int cpu, call_single_data_t *csd);
*** kernel/debug/debug_core.c:
kgdb_roundup_cpus[272]         ret = smp_call_function_single_async(cpu, csd);
*** net/core/dev.c:
net_rps_send_ipi[5818]         smp_call_function_single_async(remsd->cpu, &remsd->csd);

=================================================

For (1): These probably justify more on that we might want a patch
         like this to avoid reimplementing it everywhere.

For (2): If I read it right, smp_call_function_single_async() is the
         only place where we take a call_single_data_t structure
         rather than the (smp_call_func_t, void *) tuple.  I could
         miss something important, but otherwise I think it would be
         good to use the tuple for smp_call_function_single_async() as
         well, then we move call_single_data_t out of global header
         but move into smp.c to avoid callers from toucing it (which
         could be error-prone).  In other words, IMHO it would be good
         to have all these callers fixed.

For (3): I didn't dig, but I think some of them (or future users)
         could still suffer from the same issue on retriggering the
         WARN_ON... 

Thoughts?

Thanks,

-- 
Peter Xu

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Zijlstra]

On Wed, Dec 11, 2019 at 11:29:25AM -0500, Peter Xu wrote:
> This is also true.
> 
> Here's the statistics I mentioned:
> 
> =================================================
> 
> (1) Implemented the same counter mechanism on the caller's:
> 
> *** arch/mips/kernel/smp.c:
> tick_broadcast[713]            smp_call_function_single_async(cpu, csd);
> *** drivers/cpuidle/coupled.c:
> cpuidle_coupled_poke[336]      smp_call_function_single_async(cpu, csd);
> *** kernel/sched/core.c:
> hrtick_start[298]              smp_call_function_single_async(cpu_of(rq), &rq->hrtick_csd);
> 
> (2) Cleared the csd flags before calls:
> 
> *** arch/s390/pci/pci_irq.c:
> zpci_handle_fallback_irq[185]  smp_call_function_single_async(cpu, &cpu_data->csd);
> *** block/blk-mq.c:
> __blk_mq_complete_request[622] smp_call_function_single_async(ctx->cpu, &rq->csd);
> *** block/blk-softirq.c:
> raise_blk_irq[70]              smp_call_function_single_async(cpu, data);
> *** drivers/net/ethernet/cavium/liquidio/lio_core.c:
> liquidio_napi_drv_callback[735] smp_call_function_single_async(droq->cpu_id, csd);
> 
> (3) Others:
> 
> *** arch/mips/kernel/process.c:
> raise_backtrace[713]           smp_call_function_single_async(cpu, csd);

per-cpu csd data, seems perfectly fine usage.

> *** arch/x86/kernel/cpuid.c:
> cpuid_read[85]                 err = smp_call_function_single_async(cpu, &csd);
> *** arch/x86/lib/msr-smp.c:
> rdmsr_safe_on_cpu[182]         err = smp_call_function_single_async(cpu, &csd);

These two have csd on stack and wait with a completion. seems fine.

> *** include/linux/smp.h:
> bool[60]                       int smp_call_function_single_async(int cpu, call_single_data_t *csd);

this is the declaration, your grep went funny

> *** kernel/debug/debug_core.c:
> kgdb_roundup_cpus[272]         ret = smp_call_function_single_async(cpu, csd);
> *** net/core/dev.c:
> net_rps_send_ipi[5818]         smp_call_function_single_async(remsd->cpu, &remsd->csd);

Both percpu again.

> 
> =================================================
> 
> For (1): These probably justify more on that we might want a patch
>          like this to avoid reimplementing it everywhere.

I can't quite parse that, but if you're saying we should fix the
callers, then I agree.

> For (2): If I read it right, smp_call_function_single_async() is the
>          only place where we take a call_single_data_t structure
>          rather than the (smp_call_func_t, void *) tuple.

That's on purpose; by supplying csd we allow explicit concurrency. If
you do as proposed here:

>		I could
>          miss something important, but otherwise I think it would be
>          good to use the tuple for smp_call_function_single_async() as
>          well, then we move call_single_data_t out of global header
>          but move into smp.c to avoid callers from toucing it (which
>          could be error-prone).  In other words, IMHO it would be good
>          to have all these callers fixed.

Then you could only ever have 1 of then in flight at the same time.
Which would break things.

> For (3): I didn't dig, but I think some of them (or future users)
>          could still suffer from the same issue on retriggering the
>          WARN_ON... 

They all seem fine.

So I'm thinking your patch is good, but please also fix all 1).

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Xu]

On Mon, Dec 16, 2019 at 09:37:05PM +0100, Peter Zijlstra wrote:
> On Wed, Dec 11, 2019 at 11:29:25AM -0500, Peter Xu wrote:
> > This is also true.
> > 
> > Here's the statistics I mentioned:
> > 
> > =================================================
> > 
> > (1) Implemented the same counter mechanism on the caller's:
> > 
> > *** arch/mips/kernel/smp.c:
> > tick_broadcast[713]            smp_call_function_single_async(cpu, csd);
> > *** drivers/cpuidle/coupled.c:
> > cpuidle_coupled_poke[336]      smp_call_function_single_async(cpu, csd);
> > *** kernel/sched/core.c:
> > hrtick_start[298]              smp_call_function_single_async(cpu_of(rq), &rq->hrtick_csd);
> > 
> > (2) Cleared the csd flags before calls:
> > 
> > *** arch/s390/pci/pci_irq.c:
> > zpci_handle_fallback_irq[185]  smp_call_function_single_async(cpu, &cpu_data->csd);
> > *** block/blk-mq.c:
> > __blk_mq_complete_request[622] smp_call_function_single_async(ctx->cpu, &rq->csd);
> > *** block/blk-softirq.c:
> > raise_blk_irq[70]              smp_call_function_single_async(cpu, data);
> > *** drivers/net/ethernet/cavium/liquidio/lio_core.c:
> > liquidio_napi_drv_callback[735] smp_call_function_single_async(droq->cpu_id, csd);
> > 
> > (3) Others:
> > 
> > *** arch/mips/kernel/process.c:
> > raise_backtrace[713]           smp_call_function_single_async(cpu, csd);
> 
> per-cpu csd data, seems perfectly fine usage.

I'm not sure whether I get the point, I just feel like it could still
trigger as long as we do it super fast, before IPI handled,
disregarding whether it's per-cpu csd or not.

> 
> > *** arch/x86/kernel/cpuid.c:
> > cpuid_read[85]                 err = smp_call_function_single_async(cpu, &csd);
> > *** arch/x86/lib/msr-smp.c:
> > rdmsr_safe_on_cpu[182]         err = smp_call_function_single_async(cpu, &csd);
> 
> These two have csd on stack and wait with a completion. seems fine.

Yeh this is true, then I'm confused why they don't use the sync()
helpers..

> 
> > *** include/linux/smp.h:
> > bool[60]                       int smp_call_function_single_async(int cpu, call_single_data_t *csd);
> 
> this is the declaration, your grep went funny
> 
> > *** kernel/debug/debug_core.c:
> > kgdb_roundup_cpus[272]         ret = smp_call_function_single_async(cpu, csd);
> > *** net/core/dev.c:
> > net_rps_send_ipi[5818]         smp_call_function_single_async(remsd->cpu, &remsd->csd);
> 
> Both percpu again.
> 
> > 
> > =================================================
> > 
> > For (1): These probably justify more on that we might want a patch
> >          like this to avoid reimplementing it everywhere.
> 
> I can't quite parse that, but if you're saying we should fix the
> callers, then I agree.
> 
> > For (2): If I read it right, smp_call_function_single_async() is the
> >          only place where we take a call_single_data_t structure
> >          rather than the (smp_call_func_t, void *) tuple.
> 
> That's on purpose; by supplying csd we allow explicit concurrency. If
> you do as proposed here:
> 
> >		I could
> >          miss something important, but otherwise I think it would be
> >          good to use the tuple for smp_call_function_single_async() as
> >          well, then we move call_single_data_t out of global header
> >          but move into smp.c to avoid callers from toucing it (which
> >          could be error-prone).  In other words, IMHO it would be good
> >          to have all these callers fixed.
> 
> Then you could only ever have 1 of then in flight at the same time.
> Which would break things.

Sorry, I think you are right.

> 
> > For (3): I didn't dig, but I think some of them (or future users)
> >          could still suffer from the same issue on retriggering the
> >          WARN_ON... 
> 
> They all seem fine.
> 
> So I'm thinking your patch is good, but please also fix all 1).

Sure.  Thanks,

-- 
Peter Xu

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Zijlstra]

On Mon, Dec 16, 2019 at 03:58:33PM -0500, Peter Xu wrote:
> On Mon, Dec 16, 2019 at 09:37:05PM +0100, Peter Zijlstra wrote:
> > On Wed, Dec 11, 2019 at 11:29:25AM -0500, Peter Xu wrote:

> > > (3) Others:
> > > 
> > > *** arch/mips/kernel/process.c:
> > > raise_backtrace[713]           smp_call_function_single_async(cpu, csd);
> > 
> > per-cpu csd data, seems perfectly fine usage.
> 
> I'm not sure whether I get the point, I just feel like it could still
> trigger as long as we do it super fast, before IPI handled,
> disregarding whether it's per-cpu csd or not.

No, I wasn't paying attention last night. I'm thinking this one might
maybe be in 1). It does the state check using that bitmap.

> > > *** arch/x86/kernel/cpuid.c:
> > > cpuid_read[85]                 err = smp_call_function_single_async(cpu, &csd);
> > > *** arch/x86/lib/msr-smp.c:
> > > rdmsr_safe_on_cpu[182]         err = smp_call_function_single_async(cpu, &csd);
> > 
> > These two have csd on stack and wait with a completion. seems fine.
> 
> Yeh this is true, then I'm confused why they don't use the sync()
> helpers..

I suspect to be nice for virt. Both CPUID and MSR accesses can trap. but
now I'm confused, because it is mostly WRMSR that traps.

Anyway, see the commit here: 07cde313b2d2 ("x86/msr: Allow rdmsr_safe_on_cpu() to schedule")

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Xu]

On Tue, Dec 17, 2019 at 10:51:56AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 16, 2019 at 03:58:33PM -0500, Peter Xu wrote:
> > On Mon, Dec 16, 2019 at 09:37:05PM +0100, Peter Zijlstra wrote:
> > > On Wed, Dec 11, 2019 at 11:29:25AM -0500, Peter Xu wrote:
> 
> > > > (3) Others:
> > > > 
> > > > *** arch/mips/kernel/process.c:
> > > > raise_backtrace[713]           smp_call_function_single_async(cpu, csd);
> > > 
> > > per-cpu csd data, seems perfectly fine usage.
> > 
> > I'm not sure whether I get the point, I just feel like it could still
> > trigger as long as we do it super fast, before IPI handled,
> > disregarding whether it's per-cpu csd or not.
> 
> No, I wasn't paying attention last night. I'm thinking this one might
> maybe be in 1). It does the state check using that bitmap.

Indeed.  Though I'm not very certain to change this one too, since I'm
not sure whether that pr_warn is really intended:

        if (cpumask_test_and_set_cpu(cpu, &backtrace_csd_busy)) {
                pr_warn("Unable to send backtrace IPI to CPU%u - perhaps it hung?\n",
                        cpu);
                continue;
        }

I mean, that should depend on if it can really hang somehow (or it's
the same issue as what we're trying to fix)...  If it won't hang, then
it should be safe I think, and this pr_warn could be helpless after all.

> 
> > > > *** arch/x86/kernel/cpuid.c:
> > > > cpuid_read[85]                 err = smp_call_function_single_async(cpu, &csd);
> > > > *** arch/x86/lib/msr-smp.c:
> > > > rdmsr_safe_on_cpu[182]         err = smp_call_function_single_async(cpu, &csd);
> > > 
> > > These two have csd on stack and wait with a completion. seems fine.
> > 
> > Yeh this is true, then I'm confused why they don't use the sync()
> > helpers..
> 
> I suspect to be nice for virt. Both CPUID and MSR accesses can trap. but
> now I'm confused, because it is mostly WRMSR that traps.
> 
> Anyway, see the commit here: 07cde313b2d2 ("x86/msr: Allow rdmsr_safe_on_cpu() to schedule")

Yes that makes sense.  Thanks for the pointer.

However, then my next confusion is why they can't provide a common
solution to the smp code again... I feel like it could be even easier
(please see below).  I'm not very familiar with smp code yet, but if
it works it should benefit all callers imho.

diff --git a/kernel/smp.c b/kernel/smp.c
index dd31e8228218..7a1b163d1e4b 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -307,11 +307,12 @@ int smp_call_function_single(int cpu, smp_call_func_t func, void *info,
 
        err = generic_exec_single(cpu, csd, func, info);
 
+       put_cpu();
+
+       /* If wait, csd is on stack so it's safe without get_cpu() */
        if (wait)
                csd_lock_wait(csd);
 
-       put_cpu();
-
        return err;
 }
 EXPORT_SYMBOL(smp_call_function_single);

Thanks,

-- 
Peter Xu

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Zijlstra]

On Tue, Dec 17, 2019 at 10:31:28AM -0500, Peter Xu wrote:
> On Tue, Dec 17, 2019 at 10:51:56AM +0100, Peter Zijlstra wrote:
> > On Mon, Dec 16, 2019 at 03:58:33PM -0500, Peter Xu wrote:
> > > On Mon, Dec 16, 2019 at 09:37:05PM +0100, Peter Zijlstra wrote:
> > > > On Wed, Dec 11, 2019 at 11:29:25AM -0500, Peter Xu wrote:
> > 
> > > > > (3) Others:
> > > > > 
> > > > > *** arch/mips/kernel/process.c:
> > > > > raise_backtrace[713]           smp_call_function_single_async(cpu, csd);
> > > > 
> > > > per-cpu csd data, seems perfectly fine usage.
> > > 
> > > I'm not sure whether I get the point, I just feel like it could still
> > > trigger as long as we do it super fast, before IPI handled,
> > > disregarding whether it's per-cpu csd or not.
> > 
> > No, I wasn't paying attention last night. I'm thinking this one might
> > maybe be in 1). It does the state check using that bitmap.
> 
> Indeed.  Though I'm not very certain to change this one too, since I'm
> not sure whether that pr_warn is really intended:
> 
>         if (cpumask_test_and_set_cpu(cpu, &backtrace_csd_busy)) {
>                 pr_warn("Unable to send backtrace IPI to CPU%u - perhaps it hung?\n",
>                         cpu);
>                 continue;
>         }
> 
> I mean, that should depend on if it can really hang somehow (or it's
> the same issue as what we're trying to fix)...  If it won't hang, then
> it should be safe I think, and this pr_warn could be helpless after all.

Yeah, leave it.

> > I suspect to be nice for virt. Both CPUID and MSR accesses can trap. but
> > now I'm confused, because it is mostly WRMSR that traps.
> > 
> > Anyway, see the commit here: 07cde313b2d2 ("x86/msr: Allow rdmsr_safe_on_cpu() to schedule")
> 
> Yes that makes sense.  Thanks for the pointer.
> 
> However, then my next confusion is why they can't provide a common
> solution to the smp code again... I feel like it could be even easier
> (please see below).  I'm not very familiar with smp code yet, but if
> it works it should benefit all callers imho.

Ah, so going to sleep on wait_for_completion() is _much_ more expensive
than a short spin. So it all depends on the expected behaviour of the
IPI I suppose.

In general we expect these IPIs to be 'quick'.

Also, as is, you're allowed to use the smp_call_function*() family with
preemption disabled, which pretty much precludes using
wait_for_completion().

Re: [PATCH] smp: Allow smp_call_function_single_async() to insert locked csd

[Peter Xu]

On Tue, Dec 17, 2019 at 09:23:52PM +0100, Peter Zijlstra wrote:
> > > I suspect to be nice for virt. Both CPUID and MSR accesses can trap. but
> > > now I'm confused, because it is mostly WRMSR that traps.
> > > 
> > > Anyway, see the commit here: 07cde313b2d2 ("x86/msr: Allow rdmsr_safe_on_cpu() to schedule")
> > 
> > Yes that makes sense.  Thanks for the pointer.
> > 
> > However, then my next confusion is why they can't provide a common
> > solution to the smp code again... I feel like it could be even easier
> > (please see below).  I'm not very familiar with smp code yet, but if
> > it works it should benefit all callers imho.
> 
> Ah, so going to sleep on wait_for_completion() is _much_ more expensive
> than a short spin. So it all depends on the expected behaviour of the
> IPI I suppose.
> 
> In general we expect these IPIs to be 'quick'.
> 
> Also, as is, you're allowed to use the smp_call_function*() family with
> preemption disabled, which pretty much precludes using
> wait_for_completion().

Yes you are right, thanks!

Though I feel like previous small patch could still be an small
enhancement in that it at least shortened the get_cpu() period of
smp_call_function_single() without losing anything - so that could
still give the caller a chance to be scheduled out of the spinning at
some point when the IPI conditionally takes time to finish, imho.

-- 
Peter Xu