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

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

[Bob Liu]

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.
Thanks!

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[KOSAKI Motohiro]

> 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.
> Thanks!

I think current code is intentional. On modern cpu, bit-or is faster than
logical or.

Do you have opposite benchmark number result?


> 
> 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
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Bob Liu]

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.
>> 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.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Hugh Dickins]

[-- 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