Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753647AbbDAOWo (ORCPT ); Wed, 1 Apr 2015 10:22:44 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:35181 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753135AbbDAOWn (ORCPT ); Wed, 1 Apr 2015 10:22:43 -0400 Date: Wed, 1 Apr 2015 16:22:39 +0200 From: Frederic Weisbecker To: Linus Torvalds Cc: Rafael David Tinoco , LKML , Thomas Gleixner , Jens Axboe Subject: Re: smp_call_function_single lockups Message-ID: <20150401142238.GA5121@lerouge> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5129 Lines: 162 On Wed, Feb 11, 2015 at 12:42:10PM -0800, Linus Torvalds wrote: > [ Added Frederic to the cc, since he's touched this file/area most ] > > On Wed, Feb 11, 2015 at 11:59 AM, Linus Torvalds > wrote: > > > > So the caller has a really hard time guaranteeing that CSD_LOCK isn't > > set. And if the call is done in interrupt context, for all we know it > > is interrupting the code that is going to clear CSD_LOCK, so CSD_LOCK > > will never be cleared at all, and csd_lock() will wait forever. > > > > So I actually think that for the async case, we really *should* unlock > > before doing the callback (which is what Thomas' old patch did). > > > > And we migth well be better off doing something like > > > > WARN_ON_ONCE(csd->flags & CSD_LOCK); > > > > in smp_call_function_single_async(), because that really is a hard requirement. > > > > And it strikes me that hrtick_csd is one of these cases that do this > > with interrupts disabled, and use the callback for serialization. So I > > really wonder if this is part of the problem.. > > > > Thomas? Am I missing something? > > Ok, this is a more involved patch than I'd like, but making the > *caller* do all the CSD maintenance actually cleans things up. > > And this is still completely untested, and may be entirely buggy. What > do you guys think? > > Linus > kernel/smp.c | 78 ++++++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 47 insertions(+), 31 deletions(-) > > diff --git a/kernel/smp.c b/kernel/smp.c > index f38a1e692259..2aaac2c47683 100644 > --- a/kernel/smp.c > +++ b/kernel/smp.c > @@ -19,7 +19,7 @@ > > enum { > CSD_FLAG_LOCK = 0x01, > - CSD_FLAG_WAIT = 0x02, > + CSD_FLAG_SYNCHRONOUS = 0x02, > }; > > struct call_function_data { > @@ -107,7 +107,7 @@ void __init call_function_init(void) > */ > static void csd_lock_wait(struct call_single_data *csd) > { > - while (csd->flags & CSD_FLAG_LOCK) > + while (smp_load_acquire(&csd->flags) & CSD_FLAG_LOCK) > cpu_relax(); > } > > @@ -121,19 +121,17 @@ static void csd_lock(struct call_single_data *csd) > * to ->flags with any subsequent assignments to other > * fields of the specified call_single_data structure: > */ > - smp_mb(); > + smp_wmb(); > } > > static void csd_unlock(struct call_single_data *csd) > { > - WARN_ON((csd->flags & CSD_FLAG_WAIT) && !(csd->flags & CSD_FLAG_LOCK)); > + WARN_ON(!(csd->flags & CSD_FLAG_LOCK)); > > /* > * ensure we're all done before releasing data: > */ > - smp_mb(); > - > - csd->flags &= ~CSD_FLAG_LOCK; > + smp_store_release(&csd->flags, 0); > } > > static DEFINE_PER_CPU_SHARED_ALIGNED(struct call_single_data, csd_data); > @@ -144,13 +142,16 @@ static DEFINE_PER_CPU_SHARED_ALIGNED(struct call_single_data, csd_data); > * ->func, ->info, and ->flags set. > */ > static int generic_exec_single(int cpu, struct call_single_data *csd, > - smp_call_func_t func, void *info, int wait) > + smp_call_func_t func, void *info) > { > - struct call_single_data csd_stack = { .flags = 0 }; > - unsigned long flags; > - > - > if (cpu == smp_processor_id()) { > + unsigned long flags; > + > + /* > + * We can unlock early even for the synchronous on-stack case, > + * since we're doing this from the same CPU.. > + */ > + csd_unlock(csd); > local_irq_save(flags); > func(info); > local_irq_restore(flags); > @@ -161,21 +162,9 @@ static int generic_exec_single(int cpu, struct call_single_data *csd, > if ((unsigned)cpu >= nr_cpu_ids || !cpu_online(cpu)) > return -ENXIO; > > - > - if (!csd) { > - csd = &csd_stack; > - if (!wait) > - csd = this_cpu_ptr(&csd_data); > - } > - > - csd_lock(csd); > - > csd->func = func; > csd->info = info; > > - if (wait) > - csd->flags |= CSD_FLAG_WAIT; > - > /* > * The list addition should be visible before sending the IPI > * handler locks the list to pull the entry off it because of > @@ -190,9 +179,6 @@ static int generic_exec_single(int cpu, struct call_single_data *csd, > if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu))) > arch_send_call_function_single_ipi(cpu); > > - if (wait) > - csd_lock_wait(csd); > - > return 0; > } > > @@ -250,8 +236,17 @@ static void flush_smp_call_function_queue(bool warn_cpu_offline) > } > > llist_for_each_entry_safe(csd, csd_next, entry, llist) { > - csd->func(csd->info); > - csd_unlock(csd); > + smp_call_func_t func = csd->func; > + void *info = csd->info; > + > + /* Do we wait until *after* callback? */ > + if (csd->flags & CSD_FLAG_SYNCHRONOUS) { > + func(info); > + csd_unlock(csd); > + } else { > + csd_unlock(csd); > + func(info); Just to clarify things, the expected kind of lockup it is expected to fix is the case where the IPI is resent from the IPI itself, right? Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/