* [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check
@ 2010-01-26 6:56 Bob Liu
2010-01-26 7:00 ` KOSAKI Motohiro
0 siblings, 1 reply; 4+ messages in thread
From: Bob Liu @ 2010-01-26 6:56 UTC (permalink / raw)
To: akpm; +Cc: linux-mm, hugh.dickins, nickpiggin
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>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check
2010-01-26 6:56 [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check Bob Liu
@ 2010-01-26 7:00 ` KOSAKI Motohiro
2010-01-26 8:20 ` Bob Liu
0 siblings, 1 reply; 4+ messages in thread
From: KOSAKI Motohiro @ 2010-01-26 7:00 UTC (permalink / raw)
To: Bob Liu; +Cc: kosaki.motohiro, akpm, linux-mm, hugh.dickins, nickpiggin
> 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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check
2010-01-26 7:00 ` KOSAKI Motohiro
@ 2010-01-26 8:20 ` Bob Liu
2010-01-27 16:52 ` Hugh Dickins
0 siblings, 1 reply; 4+ messages in thread
From: Bob Liu @ 2010-01-26 8:20 UTC (permalink / raw)
To: KOSAKI Motohiro; +Cc: akpm, linux-mm, hugh.dickins, nickpiggin
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>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check
2010-01-26 8:20 ` Bob Liu
@ 2010-01-27 16:52 ` Hugh Dickins
0 siblings, 0 replies; 4+ messages in thread
From: Hugh Dickins @ 2010-01-27 16:52 UTC (permalink / raw)
To: Bob Liu; +Cc: KOSAKI Motohiro, akpm, linux-mm, nickpiggin
[-- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-01-27 16:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-26 6:56 [PATCH] page_alloc: change bit ops 'or' to logical ops in free/new page check Bob Liu
2010-01-26 7:00 ` KOSAKI Motohiro
2010-01-26 8:20 ` Bob Liu
2010-01-27 16:52 ` Hugh Dickins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).