Re: [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check

[Hugh Dickins <hugh.dickins@tiscali.co.uk>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3772 bytes --]

On Tue, 26 Jan 2010, Bob Liu wrote:
> On Tue, Jan 26, 2010 at 3:00 PM, KOSAKI Motohiro
> <kosaki.motohiro@jp.fujitsu.com> wrote:
> >> Using logical 'or' in  function free_page_mlock() and
> >> check_new_page() makes code clear and
> >> sometimes more effective (Because it can ignore other condition
> >> compare if the first condition
> >> is already true).
> >>
> >> It's Nick's patch "mm: microopt conditions" changed it from logical
> >> ops to bit ops.
> >> Maybe I didn't consider something. If so, please let me know and just
> >> ignore this patch.

Yes, please do ignore (unless you've found that logicals and branches
are actually now more efficient than the bitwises on recent processors).

> >> Thanks!
> >
> > I think current code is intentional. On modern cpu, bit-or is faster than
> > logical or.
> 
> Hmm, but if use logical ops it can be optimized by the compiler.
> In this situation, eg, if page_mapcount(page) is true, then other comparetion
> including atomic_read() willn't be called anymore.
> If use bit ops, atomic_read() and other comparetion will still be called.

In many contexts that would be a valid point to make.  But please look at
what these checks are about.  999999999 times out of a billion every one of
those tests has to be made, as efficiently as possible.  You're asking to
optimize for when memory corruption or whatever has made one condition true
which should never be true.

Hugh

> 
> I am not sure whether cpu and compiler will optimize it like the
> logical bit ops.
> If there will, the current code is intertional, else i think the
> logical ops is better.
> thanks!
> 
> -       if (unlikely(page_mapcount(page) |
> -               (page->mapping != NULL)  |
> -               (atomic_read(&page->_count) != 0) |
> +       if (unlikely(page_mapcount(page) ||
> +               (page->mapping != NULL)  ||
> +               (atomic_read(&page->_count) != 0) ||
>                (page->flags & PAGE_FLAGS_CHECK_AT_FREE))) {
> 
> 
> >
> > Do you have opposite benchmark number result?
> >
> 
> I haven't now :-).  I will test it when I have enough time.
> 
> >
> >>
> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >> ---
> >>
> >> diff --git mm/page_alloc.c mm/page_alloc.c
> >> index 05ae4e0..91ece14 100644
> >> --- mm/page_alloc.c
> >> +++ mm/page_alloc.c
> >> @@ -500,9 +500,9 @@ static inline void free_page_mlock(struct page *page)
> >>
> >>  static inline int free_pages_check(struct page *page)
> >>  {
> >> -       if (unlikely(page_mapcount(page) |
> >> -               (page->mapping != NULL)  |
> >> -               (atomic_read(&page->_count) != 0) |
> >> +       if (unlikely(page_mapcount(page) ||
> >> +               (page->mapping != NULL)  ||
> >> +               (atomic_read(&page->_count) != 0) ||
> >>                 (page->flags & PAGE_FLAGS_CHECK_AT_FREE))) {
> >>                 bad_page(page);
> >>                 return 1;
> >> @@ -671,9 +671,9 @@ static inline void expand(struct zone *zone, struct page *pa
> >>   */
> >>  static inline int check_new_page(struct page *page)
> >>  {
> >> -       if (unlikely(page_mapcount(page) |
> >> -               (page->mapping != NULL)  |
> >> -               (atomic_read(&page->_count) != 0)  |
> >> +       if (unlikely(page_mapcount(page) ||
> >> +               (page->mapping != NULL)  ||
> >> +               (atomic_read(&page->_count) != 0)  ||
> >>                 (page->flags & PAGE_FLAGS_CHECK_AT_PREP))) {
> >>                 bad_page(page);
> >>                 return 1;
> >>
> 
> -- 
> Regards,
> -Bob Liu