On Sun 27-04-14 21:36:24, Jianyu Zhan wrote:
> In put_comound_page(), we call compound_head() after !PageTail
> check fails, so in compound_head() PageTail is quite likely to
> be true, but instead it is checked with:
>
> if (unlikely(PageTail(page)))
>
> in this case, this unlikely macro is a negative hint for compiler.
>
> So this patch introduce compound_head_by_tail() which deal with
> a possible tail page(though it could be spilt by a racy thread),
> and make compound_head() a wrapper on it.
>
> Signed-off-by: Jianyu Zhan <nasa4836@xxxxxxxxx>
I really fail to see how that helps. compound_head is inlined and the
compiler should be clever enough to optimize the code properly. I
haven't tried that to be honest but this looks like it only adds a code
without any good reason. And I really hate the new name as well. What
does it suppose to mean?
> ---
> include/linux/mm.h | 34 ++++++++++++++++++++++------------
> mm/swap.c | 2 +-
> 2 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index bf9811e..1bc7baf 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -405,20 +405,30 @@ static inline void compound_unlock_irqrestore(struct page *page,
> #endif
> }
>
> +/**
> + * Note: this function must be called on a possible tail page,
> + * this tail page may not be tail anymore upon we calling this funciton,
> + * because we may race with __split_huge_page_refcount tearing down it.
> + */
> +static inline struct page *compound_head_by_tail(struct page *page)
> +{
> + struct page *head = page->first_page;
> +
> + /*
> + * page->first_page may be a dangling pointer to an old
> + * compound page, so recheck that it is still a tail
> + * page before returning.
> + */
> + smp_rmb();
> + if (likely(PageTail(page)))
> + return head;
> + return page;
> +}
> +
> static inline struct page *compound_head(struct page *page)
> {
> - if (unlikely(PageTail(page))) {
> - struct page *head = page->first_page;
> -
> - /*
> - * page->first_page may be a dangling pointer to an old
> - * compound page, so recheck that it is still a tail
> - * page before returning.
> - */
> - smp_rmb();
> - if (likely(PageTail(page)))
> - return head;
> - }
> + if (unlikely(PageTail(page)))
> + return compound_head_by_tail(page);
> return page;
> }
>
> diff --git a/mm/swap.c b/mm/swap.c
> index 0d8d891..0b05355 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -256,7 +256,7 @@ static void put_compound_page(struct page *page)
> * Case 3 is possible, as we may race with
> * __split_huge_page_refcount tearing down a THP page.
> */
> - head_page = compound_head(page);
> + head_page = compound_head_by_tail(page);
> if (!__compound_tail_refcounted(head_page))
> put_unrefcounted_compound_page(head_page, page);
> else
> --
> 2.0.0-rc1
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>