[PATCH v2] mm/page_alloc: remove unnecessary order check in __alloc_pages_direct_compact

[PATCH v2] mm/page_alloc: remove unnecessary order check in __alloc_pages_direct_compact

[Ganesh Mahendran]

In the callee try_to_compact_pages(), the (order == 0) is checked,
so remove check in __alloc_pages_direct_compact.

Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
---
v2:
  remove the check in __alloc_pages_direct_compact - Anshuman Khandual
---
 mm/page_alloc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b9ea618..2f5a82a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3173,9 +3173,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	struct page *page;
 	int contended_compaction;
 
-	if (!order)
-		return NULL;
-
 	current->flags |= PF_MEMALLOC;
 	*compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
 						mode, &contended_compaction);
-- 
1.9.1

Re: [PATCH v2] mm/page_alloc: remove unnecessary order check in __alloc_pages_direct_compact

[Balbir Singh]

On Wed, Jun 15, 2016 at 7:34 PM, Ganesh Mahendran
<opensource.ganesh@gmail.com> wrote:
> In the callee try_to_compact_pages(), the (order == 0) is checked,
> so remove check in __alloc_pages_direct_compact.
>
> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
> ---
> v2:
>   remove the check in __alloc_pages_direct_compact - Anshuman Khandual
> ---
>  mm/page_alloc.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b9ea618..2f5a82a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3173,9 +3173,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>         struct page *page;
>         int contended_compaction;
>
> -       if (!order)
> -               return NULL;
> -
>         current->flags |= PF_MEMALLOC;
>         *compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>                                                 mode, &contended_compaction);

What is the benefit of this. Is an if check more expensive than
calling the function and returning from it? I don't feel strongly
about such changes, but its good to audit the overall code for reading
and performance.

Balbir Singh

Re: [PATCH v2] mm/page_alloc: remove unnecessary order check in __alloc_pages_direct_compact

[Vlastimil Babka]

On 06/15/2016 11:40 AM, Balbir Singh wrote:
> On Wed, Jun 15, 2016 at 7:34 PM, Ganesh Mahendran
> <opensource.ganesh@gmail.com> wrote:
>> In the callee try_to_compact_pages(), the (order == 0) is checked,
>> so remove check in __alloc_pages_direct_compact.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>> ---
>> v2:
>>   remove the check in __alloc_pages_direct_compact - Anshuman Khandual
>> ---
>>  mm/page_alloc.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index b9ea618..2f5a82a 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3173,9 +3173,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>>         struct page *page;
>>         int contended_compaction;
>>
>> -       if (!order)
>> -               return NULL;
>> -
>>         current->flags |= PF_MEMALLOC;
>>         *compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>>                                                 mode, &contended_compaction);
>
> What is the benefit of this. Is an if check more expensive than
> calling the function and returning from it? I don't feel strongly
> about such changes, but its good to audit the overall code for reading
> and performance.

Agree. The majority of calls should be for order == 0 where the check 
avoids us from modifying current->flags and calling into compaction.c 
just to return and modify the flags back. I would argue that we should 
even check order before calling __alloc_pages_direct_compact() to avoid 
another potential call, but the compiler might be doing the right thing 
already.

So v1 was better in this aspect. But it wouldn't gain us any measurable 
performance benefit anyway, so we might as well leave it.

> Balbir Singh
>

Re: [PATCH v2] mm/page_alloc: remove unnecessary order check in __alloc_pages_direct_compact

[Michal Nazarewicz]

On Wed, Jun 15 2016, Balbir Singh wrote:
> On Wed, Jun 15, 2016 at 7:34 PM, Ganesh Mahendran
> <opensource.ganesh@gmail.com> wrote:
>> In the callee try_to_compact_pages(), the (order == 0) is checked,
>> so remove check in __alloc_pages_direct_compact.
>>
>> Signed-off-by: Ganesh Mahendran <opensource.ganesh@gmail.com>
>> ---
>> v2:
>>   remove the check in __alloc_pages_direct_compact - Anshuman Khandual
>> ---
>>  mm/page_alloc.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index b9ea618..2f5a82a 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3173,9 +3173,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>>         struct page *page;
>>         int contended_compaction;
>>
>> -       if (!order)
>> -               return NULL;
>> -
>>         current->flags |= PF_MEMALLOC;
>>         *compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>>                                                 mode, &contended_compaction);
>
> What is the benefit of this. Is an if check more expensive than
> calling the function and returning from it? I don't feel strongly
> about such changes, but its good to audit the overall code for reading
> and performance.

It’s a slow path so it probably doesn’t matter much.  But I also don’t
see whether this improves readability of the code.

For performance, I would rather wait for gcc to compile kernel as one
translation unit which will allow it to inline try_to_compact_pages and
notice redundant order==0 check.

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»