Messages in this thread | | | Date | Tue, 17 Dec 2013 17:53:35 +0100 | From | Oleg Nesterov <> | Subject | Re: PATCH? introduce get_compound_page (Was: process 'stuck' at exit) |
| |
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?
Oleg.
| |