Re: [PATCH] MIPS: ASID conflict after CPU hotplug

["Kevin D. Kissell" <kevink@paralogos.com>]

On 11/29/2010 7:21 PM, Maksim Rayskiy wrote:
> On Mon, Nov 29, 2010 at 6:53 PM, Kevin D. Kissell <kevink@paralogos.com> wrote:
>> Having done surgery in the past to the ASID management code, this sounds
>> like
>> a much more rational explanation of the observed problem.  Your proposed mod
>> sounds like it might work, but local_flush_tlb_mm() is implemented in terms
>> of
>> drop_mmu_context(), which only does what you want if the CPU executing the
>> code is *not* one of the CPUs participating in the memory map.  Otherwise,
>> instead of clearing the ASID in the table, it allocates a new one.  I have a
>> concern
>> that this may re-randomize things in a way that will solve your problem
>> *most*
>> of the time, but not always.
>>
> Actually, if you call this function late enough, specifically when
> cpu_online(cpu) is 0, it does exactly what I want from it - that is
> clears ASID in the context.

It does exactly what you want when cpumask_test_cpu(cpu, mm_cpumask(mm))
is false, but I don't think that will necessarily be the case when
cpu_online(cpu) is 0.
It's keying off the CPU mask in the mm struct, not the global online
mask.  If those
are actually guaranteed to be in sync, then your hack would be fine.  
I'd just beseech
you to make sure you comment the invocation of drop_mmu_context() is
commented
to the effect that "This needs to be done after the CPU is offline for
the purposes
of a cpumask check on the mm".
> I am calling it from play_dead() which is platform specific, but there
> might be a place for it in platform-independent code as well.
> Another option would be not to use drop_mmu_context() but rather clear
> the context directly, since we know exactly what we want to do at this
> point.
>
>> Now that we have a better understanding of the failure, your initial notion
>> of *not* restarting the ASID sequence on a hotplug insertion doesn't seem
>> as crazy - it's certainly the zen "doing by doing nothing" way to go,
>> without
>> the iterative overhead of walking the full process table.  But as we
>> discussed,
>> it has the downside of requiring new state infrastructure for tracking
>> hotplugs,
>> and we'd want to be sure that it's well behaved in the case where we have a
>> post-initial-boot hotplug event that brings a CPU online that has never been
>> initialized.  To take that tack, we'd need a per-CPU-slot bit which says "I
>> have
>> a valid ASID sequence, thank you", which is checked in per_cpu_trap_init()
>> (or some other appropriate hook), and the ASID "cache" is initialized only
>> if it's needed, which *might* be on a hotplug.
> You are talking about adding this bit to cpuinfo_mips, correct?

That would be a logical place to hang it, though I'm not sure if we keep
volatile data like that in the array these days.  It might need to be
lock protected.

            Regards,

            Kevin K.