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

[Bob Liu <lliubbo@gmail.com>]

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>