Re: [PATCH] x86: hold mm->page_table_lock while doing vmalloc_sync

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 02/03/2011 05:21 PM, Andrea Arcangeli wrote:
> On Thu, Feb 03, 2011 at 12:44:02PM -0800, Jeremy Fitzhardinge wrote:
>> On 02/02/2011 06:48 PM, Andrea Arcangeli wrote:
>>> Hello,
>>>
>>> Larry (CC'ed) found a problem with the patch in subject. When
>>> USE_SPLIT_PTLOCKS is not defined (NR_CPUS == 2) it will deadlock in
>>> ptep_clear_flush_notify in rmap.c because it's sending IPIs with the
>>> page_table_lock already held, and the other CPUs now spins on the
>>> page_table_lock with irq disabled, so the IPI never runs. With
>>> CONFIG_TRANSPARENT_HUGEPAGE=y this deadlocks happens even with
>>> USE_SPLIT_PTLOCKS defined so it become visible but it needs to be
>>> fixed regardless (for NR_CPUS == 2).
>> What's "it" here?  Do you mean vmalloc_sync_all?  vmalloc_sync_one?
>> What's the callchain?
> Larry just answered to that. If something is unclear let me know. I
> never reproduced it, but it also can happen without THP enabled, you
> just need to set NR_CPUS to 2 during "make menuconfig".
>
>>> spin_lock_irqsave(pgd_lock) so I guess it's either common code, or
>>> it's superfluous and not another Xen special requirement.
>> There's no special Xen requirement here.
> That was my thought too considering the other archs...
>
>> mmdrop() can be called from interrupt context, but I don't know if it
>> will ever drop the last reference from interrupt, so maybe you can get
>> away with it.
> Yes the issue is __mmdrop, so it'd be nice to figure if __mmdrop can
> also run from irq (or only mmdrop fast path which would be safe even
> without _irqsave).
>
> Is this a Xen only thing? Or is mmdrop called from regular
> linux. Considering other archs also _irqsave I assume it's common code
> calling mmdrop (otherwise it means they cut-and-pasted a Xen
> dependency). This comment doesn't really tell me much.

No, I don't think there's any xen-specific code which calls mmdrop (at
all, let alone in interrupt context).  Erm, but I'm not sure where it
does.  I had a thinko that "schedule" would be one of those places, but
calling that from interrupt context would cause much bigger problems :/
> static void pgd_dtor(pgd_t *pgd)
> {
> 	unsigned long flags; /* can be called from interrupt context 	*/
>
> 	if (SHARED_KERNEL_PMD)
> 	   return;
>
> 	   VM_BUG_ON(in_interrupt());
> 	   spin_lock(&pgd_lock);
>
> This comment tells the very __mmdrop can be called from irq context,
> not just mmdrop. But I didn't find where yet... Can you tell me?

No.  I don't think I wrote that comment.  It possibly just some ancient
lore that could have been correct at one point, or perhaps it never true.

>>> @@ -247,7 +248,7 @@ void vmalloc_sync_all(void)
>>>  			if (!ret)
>>>  				break;
>>>  		}
>>> -		spin_unlock_irqrestore(&pgd_lock, flags);
>>> +		spin_unlock(&pgd_lock, flags);
>> Urp.  Did this compile?
> Yes it builds

(spin_unlock() shouldn't take a "flags" arg.)


> I'm not reposting a version that builds for 32bit x86 too until we
> figure out the mmdrop thing...

Stick it in next and look for explosion reports?

    J