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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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 ?

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.