Re: [PATCH 3/4] sparc64: convert spinlock_t to raw_spinlock_t in mmu_context_t

[Allen Pais <allen.pais@oracle.com>]

On Wednesday 19 February 2014 02:55 PM, Kirill Tkhai wrote:
> 19.02.2014, 13:13, "Allen Pais" <allen.pais@oracle.com>:
> 
>>  On Wednesday 19 February 2014 02:27 PM, Kirill Tkhai wrote:
>>>   19.02.2014, 12:12, "Allen Pais" <allen.pais@oracle.com>:
>>>>>>    diff --git a/arch/sparc/mm/tsb.c b/arch/sparc/mm/tsb.c
>>>>>>    index 9eb10b4..24dcd29 100644
>>>>>>    --- a/arch/sparc/mm/tsb.c
>>>>>>    +++ b/arch/sparc/mm/tsb.c
>>>>>>    @@ -6,6 +6,7 @@
>>>>>>     #include <linux/kernel.h>
>>>>>>     #include <linux/preempt.h>
>>>>>>     #include <linux/slab.h>
>>>>>>    +#include <linux/locallock.h>
>>>>>>     #include <asm/page.h>
>>>>>>     #include <asm/pgtable.h>
>>>>>>     #include <asm/mmu_context.h>
>>>>>>    @@ -14,6 +15,7 @@
>>>>>>     #include <asm/oplib.h>
>>>>>>
>>>>>>     extern struct tsb swapper_tsb[KERNEL_TSB_NENTRIES];
>>>>>>    +static DEFINE_LOCAL_IRQ_LOCK(tsb_lock);
>>>>>>
>>>>>>     static inline unsigned long tsb_hash(unsigned long vaddr, unsigned long hash_sh
>>>>>>     {
>>>>>>    @@ -71,9 +73,9 @@ static void __flush_tsb_one(struct tlb_batch *tb, unsigned lon
>>>>>>     void flush_tsb_user(struct tlb_batch *tb)
>>>>>>     {
>>>>>>            struct mm_struct *mm = tb->mm;
>>>>>>    -       unsigned long nentries, base, flags;
>>>>>>    +       unsigned long nentries, base;
>>>>>>
>>>>>>    -       raw_spin_lock_irqsave(&mm->context.lock, flags);
>>>>>>    +       local_lock(tsb_lock);
>>>>>>
>>>>>>            base = (unsigned long) mm->context.tsb_block[MM_TSB_BASE].tsb;
>>>>>>            nentries = mm->context.tsb_block[MM_TSB_BASE].tsb_nentries;
>>>>>>    @@ -90,7 +92,7 @@ void flush_tsb_user(struct tlb_batch *tb)
>>>>>>                    __flush_tsb_one(tb, HPAGE_SHIFT, base, nentries);
>>>>>>            }
>>>>>>     #endif
>>>>>>    -       raw_spin_unlock_irqrestore(&mm->context.lock, flags);
>>>>>>    +       local_unlock(tsb_lock);
>>>>>    It seems to be not good for me. Tsb setup is in tsb_grow() and it must
>>>>>    be synchronized with flushing. Flushing is also being made in flush_tsb_user_page()..
>>>>>
>>>>>    Which last stack stack has you received with tb->active, permanently set to zero?
>>>>   I agree with you point about flushing in flush_tbs_user_page too. Like i said, this is
>>>>   a bit tricky to actually debug.
>>>>
>>>>   Yes, tb->active was set to zero.
>>>   If tb->active is zero, flush_tsb_user() is never called, because of tlb_nr is permanently zero.
>>  Sorry, my bad. tb->active was set to one when I ran the test with the above patch.
> 
> It seems for me it's better to decide the problem not changing protector of tsb like in patch above.
> You may get good stack without sun4v_data_access_exception error, which was in the first or second
> message.

I agree, I hope to get more a few opinions on the patches/issues. Meanwhile, Lemme see if I could
manage a way to stick to the old protectors and fix the stalls.

- Allen