Re: [RFC] ARM: lockless get_user_pages_fast()

[Will Deacon]

Hi Steve,

[adding linux-mm, since this has turned into a discussion about THP
splitting]

On Fri, Oct 04, 2013 at 11:31:42AM +0100, Steve Capper wrote:
> On Thu, Oct 03, 2013 at 11:07:44AM -0700, Zi Shen Lim wrote:
> > On Thu, Oct 3, 2013 at 10:27 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
> > > On Thu, Oct 03, 2013 at 06:15:15PM +0100, Zi Shen Lim wrote:
> > >> Futex uses GUP. Currently on ARM, the default __get_user_pages_fast
> > >> being used always returns 0, leading to a forever loop in get_futex_key :(
> > >
> > > This looks pretty much like an exact copy of the x86 version, which will
> > > likely also result in another exact copy for arm64. Can none of this code be
> > > made common? Furthermore, the fact that you've lifted the code and not
> > > provided much of an explanation in the cover letter hints that you might not
> > > be aware of all the subtleties involved here...

[...]

> > >> +static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> > >> +             int write, struct page **pages, int *nr)
> > >> +{
> > >> +     unsigned long next;
> > >> +     pmd_t *pmdp;
> > >> +
> > >> +     pmdp = pmd_offset(&pud, addr);
> > >> +     do {
> > >> +             pmd_t pmd = *pmdp;
> > >> +
> > >> +             next = pmd_addr_end(addr, end);
> > >> +             /*
> > >> +              * The pmd_trans_splitting() check below explains why
> > >> +              * pmdp_splitting_flush has to flush the tlb, to stop
> > >> +              * this gup-fast code from running while we set the
> > >> +              * splitting bit in the pmd. Returning zero will take
> > >> +              * the slow path that will call wait_split_huge_page()
> > >> +              * if the pmd is still in splitting state. gup-fast
> > >> +              * can't because it has irq disabled and
> > >> +              * wait_split_huge_page() would never return as the
> > >> +              * tlb flush IPI wouldn't run.
> > >> +              */
> > >> +             if (pmd_none(pmd) || pmd_trans_splitting(pmd))
> > >> +                     return 0;
> > >> +             if (unlikely(pmd_huge(pmd))) {
> > >> +                     if (!gup_huge_pmd(pmd, addr, next, write, pages, nr))
> > >> +                             return 0;
> > >> +             } else {
> > >> +                     if (!gup_pte_range(pmd, addr, next, write, pages, nr))
> > >> +                             return 0;
> > >> +             }
> > >> +     } while (pmdp++, addr = next, addr != end);
> > >
> > > ...case in point: we don't (usually) require IPIs to shoot down TLB entries
> > > in SMP systems, so this is racy under thp splitting.

[...]

> As Will pointed out, ARM does not usually require IPIs to shoot down TLB
> entries. So the local_irq_disable will not necessarily block pagetables being
> freed when fast_gup is running.
> 
> Transparent huge pages when splitting will set the pmd splitting bit then
> perform a tlb invalidate, then proceed with the split. Thus a splitting THP
> will not always be blocked by local_irq_disable on ARM. This does not only
> affect fast_gup, futexes are also affected. From my understanding of futex on
> THP tail case in kernel/futex.c, it looks like an assumption is made there also
> that splitting pmds can be blocked by disabling local irqs.
> 
> PowerPC and SPARC, like ARM do not necessarily require IPIs for TLB shootdown
> either so they make use of tlb_remove_table (CONFIG_HAVE_RCU_TABLE_FREE). This
> identifies pages backing pagetables that have multiple users and batches them
> up, and then performs a dummy IPI before freeing them en masse. This reduces
> the performance impact from the IPIs (by doing considerably fewer of them), and
> guarantees that pagetables cannot be freed from under the fast_gup.
> Unfortunately this also means that the fast_gup has to be aware of ptes/pmds
> changing from under it.

[...]

> There's also the possibility of blocking without an IPI, but it's not obvious
> to me how to do that (that would probably necessitate a change to
> kernel/futex.c). I've just picked this up recently and am still trying to
> understand it fully.

The IPI solution looks like a hack to me and essentially moves the
synchronisation down into the csd_lock on the splitting side as part of the
cross-call to invalidate the TLB. Furthermore, the TLB doesn't even need to
be invalidated afaict, since we're just updating software bits.

Instead, I wonder whether this can be solved with a simple atomic_t:

	- The fast GUP code (read side) does something like:

		atomic_inc(readers);
		smp_mb__after_atomic_inc();
		__get_user_pages_fast(...);
		smp_mb__before_atomic_dec();
		atomic_dec(readers);

	- The splitting code (write side) then polls the counter to reach
	  zero:

		pmd_t pmd = pmd_mksplitting(*pmdp);
		set_pmd_at(vma->vm_mm, address, pmdp, pmd);
		smp_mb();
		while (atomic_read(readers) != 0)
			cpu_relax();

that way, we don't need to worry about IPIs, we don't need to disable
interrupts on the read side and we still get away without heavyweight
locking.

Of course, I could well be missing something here, but what we currently
have in mainline doesn't work for ARM anyway (since TLB invalidation is
broadcast in hardware).

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/