Re: [PATCH 2/3] powerpc/mm: Rename find_linux_pte_or_hugepte

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Wednesday 17 May 2017 10:27 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2017-05-17 at 08:57 +0530, Aneesh Kumar K.V wrote:
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>>
>>> On Tue, 2017-05-16 at 14:56 +0530, Aneesh Kumar K.V wrote:
>>>> +static inline pte_t *find_linux_pte(pgd_t *pgdir, unsigned long ea,
>>>> +                                   bool *is_thp, unsigned *hshift)
>>>> +{
>>>> +       VM_WARN((!arch_irqs_disabled() && !__hard_irqs_disabled()) ,
>>>> +               "%s called with irq enabled\n", __func__);
>>>> +       return __find_linux_pte(pgdir, ea, is_thp, hshift);
>>>> +}
>>>> +
>>> When is arch_irqs_disabled() not sufficient ?
>> We can do lockless page table walk in interrupt handlers where we find
>> MSR_EE = 0.
> Such as ?
>
>> I was not sure we mark softenabled 0 there. What I wanted to
>> indicate in the patch is that we are safe with either softenable = 0 or MSR_EE = 0
> Reading the MSR is expensive...
>
> Can you find a case where we are hard disabled and not soft disable in
> C code ? I can't think of one off-hand ... I know we have some asm that
> can do that very temporarily but I wouldn't think we have anything at
> runtime.
>
> Talking of which, we have this in irq.c:
>
>
> #ifdef CONFIG_TRACE_IRQFLAGS
> 	else {
> 		/*
> 		 * We should already be hard disabled here. We had bugs
> 		 * where that wasn't the case so let's dbl check it and
> 		 * warn if we are wrong. Only do that when IRQ tracing
> 		 * is enabled as mfmsr() can be costly.
> 		 */
> 		if (WARN_ON(mfmsr() & MSR_EE))
> 			__hard_irq_disable();
> 	}
> #endif
>
> I think we should move that to a new CONFIG_PPC_DEBUG_LAZY_IRQ because
> distros are likely to have CONFIG_TRACE_IRQFLAGS these days no ?

Yes, CONFIG_TRACE_IRQFLAGS are enabled. So in my local_t patchset,
I have added a patch to do the same with a flag "CONFIG_IRQ_DEBUG_SUPPORT"

mpe reported boot hang with the current version of the
local_t patchset in Booke system, and have a fix for the
same and it is being tested. Will post a newer version
once the patch verified.

Maddy
>
> Also we could add additional checks, such as MSR_EE matching paca-
>> irq_happened or the above you mentioned, ie, WARN if we find case
> where IRQs are hard disabled but soft enabled.
>
> If we find these, I think we should fix them.
>
> Cheers,
> Ben.
>