On 12/11, Thomas Gleixner wrote:
>
> On Wed, 11 Dec 2013, Oleg Nesterov wrote:
> >
> > I know almost nothing about THP, but why we may need write == true in
> > this case?
> >
> > IOW,
> >
> > > - if (likely(__get_user_pages_fast(address, 1, 1, &page) == 1)) {
> > > + if (likely(__get_user_pages_fast(address, 1, !ro, &page) == 1))
> > > {
> >
> > can't
> > if (likely(__get_user_pages_fast(address, 1, 0, &page) == 1)) {
> >
> > work?
>
> If the futex call needs to write to that address, we need a writeable
> mapping, right? And get_user_pages_fast() will verify that for us.
Yes sure. I meant __get_user_pages_fast() which is called to find
page_head.
> > I have to admit, I do not understand why we can't avoid this altogether.
> >
> > __get_page_tail() can find the stable ->first_page, why get_futex_key()
> > can't ?
>
> Because it can be split under us.
I understand, but why we can't rely on compound_lock() like
__get_page_tail() does?
OK, could you please explain why something like the pseudo-code
below can't work?
Oleg.
--- x/mm/swap.c
+++ x/mm/swap.c
@@ -210,7 +210,7 @@ EXPORT_SYMBOL(put_page);
* This function is exported but must not be called by anything other
* than get_page(). It implements the slow path of get_page().
*/
-bool __get_page_tail(struct page *page)
+struct page *__get_page_tail(struct page *page)
{
/*
* This takes care of get_page() if run on a tail page
@@ -235,7 +235,7 @@ bool __get_page_tail(struct page *page)
*/
VM_BUG_ON(!PageHead(page_head));
__get_page_tail_foll(page, false);
- return true;
+ return page_head;
} else {
/*
* __split_huge_page_refcount run
@@ -247,7 +247,7 @@ bool __get_page_tail(struct page *page)
* slab on x86).
*/
put_page(page_head);
- return false;
+ return NULL;
}
}
@@ -264,10 +264,12 @@ bool __get_page_tail(struct page *page)
got = true;
}
compound_unlock_irqrestore(page_head, flags);
- if (unlikely(!got))
+ if (likely(got))
+ return page_head;
+ else
put_page(page_head);
}
- return got;
+ return NULL;
}
EXPORT_SYMBOL(__get_page_tail);
--- x/kernel/futex.c
+++ x/kernel/futex.c
@@ -282,41 +282,11 @@ again:
else
err = 0;
-#ifdef CONFIG_TRANSPARENT_HUGEPAGE
- page_head = page;
- if (unlikely(PageTail(page))) {
+ page_head = __get_page_tail(page);
+ if (page_head) {
put_page(page);
- /* serialize against __split_huge_page_splitting() */
- local_irq_disable();
- if (likely(__get_user_pages_fast(address, 1, 1, &page) == 1)) {
- page_head = compound_head(page);
- /*
- * page_head is valid pointer but we must pin
- * it before taking the PG_lock and/or
- * PG_compound_lock. The moment we re-enable
- * irqs __split_huge_page_splitting() can
- * return and the head page can be freed from
- * under us. We can't take the PG_lock and/or
- * PG_compound_lock on a page that could be
- * freed from under us.
- */
- if (page != page_head) {
- get_page(page_head);
- put_page(page);
- }
- local_irq_enable();
- } else {
- local_irq_enable();
- goto again;
- }
- }
-#else
- page_head = compound_head(page);
- if (page != page_head) {
- get_page(page_head);
- put_page(page);
- }
-#endif
+ else
+ page_head = page;
lock_page(page_head);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
