Re: PATCH? introduce get_compound_page (Was: process 'stuck' at exit)

[Andrea Arcangeli <aarcange@redhat.com>]

On Tue, Dec 17, 2013 at 05:53:35PM +0100, Oleg Nesterov wrote:
> On 12/16, Oleg Nesterov wrote:
> >
> > On 12/16, Andrea Arcangeli wrote:
> > >
> > > On Mon, Dec 16, 2013 at 07:36:18PM +0100, Oleg Nesterov wrote:
> > > >
> > > > And compound_lock_irqsave() looks racy even after get_page_unless_zero().
> > > >
> > > > For example, suppose that page_head was already freed and then re-allocated
> > > > as (say) alloc_pages(__GFP_COMP, 1). get_page_unless_zero() can succeed right
> > > > after prep_new_page() does set_page_refcounted(). Now, can't compound_lock()
> > > > race with the non-atomic prep_compound_page()->__SetPageHead() ?
> > >
> > > Yes. We need to change to:
> > >
> > > 	if (order && (gfp_flags & __GFP_COMP))
> > > 		prep_compound_page(page, order);
> > > 	smp_wmb();
> > > 	/* as the compound_lock can be taken after it's refcounted */
> > > 	set_page_refcounted(page);
> > >
> > > __SetPageHead uses bts asm insn so literally only a "lock" prefix is
> > > missing in a assembly instruction. So the race window is incredibly
> > > small, but it must be fixed indeed. This also puts set_page_refcounted
> > > as the last action of buffered_rmqueue so there shouldn't be any other
> > > issues like this left in the page allocation code.
> > >
> > > Can you reorder set_page_refcount in your v2?
> >
> > OK. I'll try to make something on Wednesday.
> 
> Yes, I will, but...
> 
> I can't stop thinking about another change. What if we simply change
> __split_huge_page_refcount() to also do compound_lock/unlock(page_tail)
> in a main loop?
> 
> This way we can greatly simplify get/put_page paths, we can rely on
> compound_lock(sub-page) and avoid get_page_unless_zero(page_head).
> Yes, this will make _split a bit slower, but PG_compound_lock should
> not be contended? And we should change page_tail->flags carefully, but
> this looks simple.
> 
> Or this is not possible/desirable?

That would be 512 nested spinlocks instead of 1, last time I did
something like that in mm_take_all_locks people weren't too pleased as
it started firing lockdeps complains too. Generally I try to avoid
taking too many locks nested if I can.

mm_take_all_locks is fine because it only runs when you register an mm
into a device driver, so it is a very rare event and not performance
critical at all, it is a slow path by all means (only runs when you
start a virtual machine or start X with nvidia etc..). So it is not a
concern. split_huge_page to the contrary could run in a flood if
you're unlucky. split_huge_page is needed not just to handle non-THP
aware paths that mostly disappeared by now, but also when you truncate
a vma so that a THP doesn't fit in it anymore. So it's up to userland
how frequently it needs to run.

I think it's reasonable to consider it though, but then it's not
guaranteed that a put_page on a THP tail is more frequent than
split_huge_page. Keep in mind we do the get_page_unless_zero to
stabilize the head to take the compound_lock on it, only for the
tails, never for the heads. So this restricts it to _only_ the
put_page following a gup_fast. Only gup_fast can ever take a reference
on the tail pages of a THP. Nothing else can.

I intend to add the foll_flags to gup_fast parameter so we remove
FOLL_GET from it in the KVM page fault to avoid doing atomic_inc
immediately followed by atomic_dec when establishing sptes. So the
only transient mappings that cannot be converted to pin-less
mmu_notifier will have to run put_page in gup_fast.

In short you're only going to help O_DIRECT with that change, and
removing 1 locked op for every 4k written to disk may not offset the
cost of 511 locked ops in split_huge_page.

Still worth thinking about it, but not obvious win in my view (plus
the lockdep trouble with taking too many locks nested). Comments welcome.

Thanks!
Andrea