Re: [PATCH RFC 2/2] mm: introdule compound_head_by_tail()

[Michal Hocko <mhocko@suse.cz>]

On Mon 28-04-14 23:53:28, Jianyu Zhan wrote:
> Hi, Michal,
> 
> On Mon, Apr 28, 2014 at 10:54 PM, Michal Hocko <mhocko@suse.cz> wrote:
> > 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?
> 
> the code in question is as below:
> 
> --- snipt ----
> if (likely(!PageTail(page))) {                  <------  (1)
>                 if (put_page_testzero(page)) {
>                         /*
>                         ¦* By the time all refcounts have been released
>                         ¦* split_huge_page cannot run anymore from under us.
>                         ¦*/
>                         if (PageHead(page))
>                                 __put_compound_page(page);
>                         else
>                                 __put_single_page(page);
>                 }
>                 return;
> }
> 
> /* __split_huge_page_refcount can run under us */
> page_head = compound_head(page);        <------------ (2)
> --- snipt ---
> 
> if at (1) ,  we fail the check, this means page is *likely* a tail page.
> 
> Then at (2), yes, compoud_head(page) is inlined, it is :
> 
> --- snipt ---
> static inline struct page *compound_head(struct page *page)
> {
>           if (unlikely(PageTail(page))) {           <----------- (3)
>               struct page *head = page->first_page;
> 
>                 smp_rmb();
>                 if (likely(PageTail(page)))
>                         return head;
>         }
>         return page;
> }
> --- snipt ---
> 
> here, the (3) unlikely in the case is  a negative hint, because it
> is *likely* a tail page. So the check (3) in this case is not good,
> so I introduce a helper for this case.
> 
> Actually, I checked the assembled code, the compiler is _not_
> so smart to recognize this case. It just does optimization as
> the hint unlikely() told it.

OK, the generated code is sligly smaller:
  11869    1328      32   13229    33ad mm/swap.o.after
  11880    1328      32   13240    33b8 mm/swap.o.before

The another question is. Does this matter? You are optimizing a slow
path which is not bad in general but it would be much better if you
show numbers which tell us that it helps noticeably in some loads or it
helped with future readability and maintainability. My experience tells
me that having very specialized helper functions used at a single place
don't help in neither in readability nor maintainability.
-- 
Michal Hocko
SUSE Labs