lkml.org 
[lkml]   [2012]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] x86, mm: only wait for flushes from online cpus
    Srivatsa S. Bhat (srivatsa.bhat@linux.vnet.ibm.com) wrote:
    > On 06/23/2012 03:36 AM, Mandeep Singh Baines wrote:
    > > A cpu in the mm_cpumask could go offline before we send the invalidate
    > > IPI causing us to wait forever. Avoid this by only waiting for online
    > > cpus.
    > >
    > > We are seeing a softlockup reporting during shutdown. The stack
    > > trace shows us that we are inside default_send_IPI_mask_logical:
    > >
    > [...]
    > > Changes in V2:
    > > * bitmap_and is not atomic so use a temporary bitmask
    > >
    >
    > Looks like I posted my reply to v1. So I'll repeat the same suggestions in
    > this thread as well.
    >
    > > ---
    > > arch/x86/mm/tlb.c | 9 ++++++++-
    > > 1 files changed, 8 insertions(+), 1 deletions(-)
    > >
    > > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
    > > index d6c0418..231a0b9 100644
    > > --- a/arch/x86/mm/tlb.c
    > > +++ b/arch/x86/mm/tlb.c
    > > @@ -185,6 +185,8 @@ static void flush_tlb_others_ipi(const struct cpumask *cpumask,
    > > f->flush_mm = mm;
    > > f->flush_va = va;
    > > if (cpumask_andnot(to_cpumask(f->flush_cpumask), cpumask, cpumask_of(smp_processor_id()))) {
    > > + DECLARE_BITMAP(tmp_cpumask, NR_CPUS);
    > > +
    > > /*
    > > * We have to send the IPI only to
    > > * CPUs affected.
    > > @@ -192,8 +194,13 @@ static void flush_tlb_others_ipi(const struct cpumask *cpumask,
    > > apic->send_IPI_mask(to_cpumask(f->flush_cpumask),
    > > INVALIDATE_TLB_VECTOR_START + sender);
    > >
    >
    > This function is always called with preempt_disabled() right?
    > In that case, _while_ this function is running, a CPU cannot go offline
    > because of stop_machine(). (I understand that it might go offline in between
    > calculating that cpumask and calling preempt_disable() - which is the race
    > you are trying to handle).
    >

    Ah. Good point. A cpu cannot be remove from the cpu_online_mask while
    preemption is disabled because stop_machine() can't run until
    preemption is enabled.

    ./kernel/cpu.c: err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));

    > So, why not take the offline cpus out of the way even before sending that IPI?
    > That way, we need not modify the while loop below.
    >

    Acked-off-by: Mandeep Singh Baines <msb@chromium.org>

    Do you mind re-sending you're patch with a proper sign-off.

    Thanks and regards,
    Mandeep

    > > - while (!cpumask_empty(to_cpumask(f->flush_cpumask)))
    > > + /* Only wait for online cpus */
    > > + do {
    > > + cpumask_and(to_cpumask(tmp_cpumask),
    > > + to_cpumask(f->flush_cpumask),
    > > + cpu_online_mask);
    > > cpu_relax();
    > > + } while (!cpumask_empty(to_cpumask(tmp_cpumask)));
    > > }
    > >
    > > f->flush_mm = NULL;
    > >
    >
    > That is, how about something like this:
    >
    > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
    > index 5e57e11..9d387a9 100644
    > --- a/arch/x86/mm/tlb.c
    > +++ b/arch/x86/mm/tlb.c
    > @@ -186,7 +186,11 @@ static void flush_tlb_others_ipi(const struct cpumask *cpumask,
    >
    > f->flush_mm = mm;
    > f->flush_va = va;
    > - if (cpumask_andnot(to_cpumask(f->flush_cpumask), cpumask, cpumask_of(smp_processor_id()))) {
    > +
    > + cpumask_and(to_cpumask(f->flush_cpumask), cpumask, cpu_online_mask);
    > + cpumask_clear_cpu(smp_processor_id(), to_cpumask(f->flush_cpumask));
    > +
    > + if (!cpumask_empty(to_cpumask(f->flush_cpumask))) {
    > /*
    > * We have to send the IPI only to
    > * CPUs affected.
    >
    >
    > Regards,
    > Srivatsa S. Bhat
    > IBM Linux Technology Center
    >


    \
     
     \ /
      Last update: 2012-07-19 00:41    [W:0.043 / U:0.596 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site