linux-kernel - Re: [PATCH RFC 2/2] mm: introdule compound_head_by

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 28 Apr 2014 20:23:21 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Jianyu Zhan <nasa4836@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	kirill.shutemov@...ux.intel.com, Rik van Riel <riel@...hat.com>,
	Jiang Liu <liuj97@...il.com>, peterz@...radead.org,
	Johannes Weiner <hannes@...xchg.org>,
	Mel Gorman <mgorman@...e.de>,
	Andrea Arcangeli <aarcange@...hat.com>, sasha.levin@...cle.com,
	liwanp@...ux.vnet.ibm.com, khalid.aziz@...cle.com,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 2/2] mm: introdule compound_head_by_tail()

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@...e.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/