[PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Zeng Tao]

This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
Add cached pool to spead up cached buffer alloc"), the gfp_mask low
order pool is overlapped by the high order inside the loop, so the
gfp_mask of all pools are set to high_order_gfp_flags.

Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
---
 drivers/staging/android/ion/ion_system_heap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
index 4dc5d7a..b6386be 100644
--- a/drivers/staging/android/ion/ion_system_heap.c
+++ b/drivers/staging/android/ion/ion_system_heap.c
@@ -298,10 +298,10 @@ static int ion_system_heap_create_pools(struct ion_page_pool **pools,
 					bool cached)
 {
 	int i;
-	gfp_t gfp_flags = low_order_gfp_flags;
 
 	for (i = 0; i < NUM_ORDERS; i++) {
 		struct ion_page_pool *pool;
+		gfp_t gfp_flags = low_order_gfp_flags;
 
 		if (orders[i] > 4)
 			gfp_flags = high_order_gfp_flags;
-- 
2.7.4

Re: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Chen Feng]

On 2018/1/9 18:43, Zeng Tao wrote:
> This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
> Add cached pool to spead up cached buffer alloc"), the gfp_mask low
> order pool is overlapped by the high order inside the loop, so the
> gfp_mask of all pools are set to high_order_gfp_flags.
> 

Thanks
> Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
> ---
>  drivers/staging/android/ion/ion_system_heap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> index 4dc5d7a..b6386be 100644
> --- a/drivers/staging/android/ion/ion_system_heap.c
> +++ b/drivers/staging/android/ion/ion_system_heap.c
> @@ -298,10 +298,10 @@ static int ion_system_heap_create_pools(struct ion_page_pool **pools,
>  					bool cached)
>  {
>  	int i;
> -	gfp_t gfp_flags = low_order_gfp_flags;
>  
>  	for (i = 0; i < NUM_ORDERS; i++) {
>  		struct ion_page_pool *pool;
> +		gfp_t gfp_flags = low_order_gfp_flags;

Not define here. Better "gfp_flags = low_order_gfp_flags"
>  
>  		if (orders[i] > 4)
>  			gfp_flags = high_order_gfp_flags;
> 

Re: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Dan Carpenter]

On Tue, Jan 09, 2018 at 11:30:09AM +0800, Chen Feng wrote:
> 
> 
> On 2018/1/9 18:43, Zeng Tao wrote:
> > This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
> > Add cached pool to spead up cached buffer alloc"),

Use the Fixes tag.

Fixes: e7f63771b60e ("ION: Sys_heap: Add cached pool to spead up cached buffer alloc")

> the gfp_mask low
> > order pool is overlapped by the high order inside the loop, so the
> > gfp_mask of all pools are set to high_order_gfp_flags.
> > 
> 
> Thanks
> > Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
> > ---
> >  drivers/staging/android/ion/ion_system_heap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
> > index 4dc5d7a..b6386be 100644
> > --- a/drivers/staging/android/ion/ion_system_heap.c
> > +++ b/drivers/staging/android/ion/ion_system_heap.c
> > @@ -298,10 +298,10 @@ static int ion_system_heap_create_pools(struct ion_page_pool **pools,
> >  					bool cached)
> >  {
> >  	int i;
> > -	gfp_t gfp_flags = low_order_gfp_flags;
> >  
> >  	for (i = 0; i < NUM_ORDERS; i++) {
> >  		struct ion_page_pool *pool;
> > +		gfp_t gfp_flags = low_order_gfp_flags;
> 
> Not define here. Better "gfp_flags = low_order_gfp_flags"

Either way is fine...

regards,
dan carpenter

答复: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Zengtao (B)]

>-----邮件原件-----
>发件人: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>发送时间: 2018年1月9日 17:14
>收件人: Chenfeng (puck) <puck.chen@hisilicon.com>
>抄送: Zengtao (B) <prime.zeng@hisilicon.com>; labbott@redhat.com;
>sumit.semwal@linaro.org; gregkh@linuxfoundation.org; arve@android.com;
>tkjos@android.com; maco@android.com; devel@driverdev.osuosl.org;
>linux-kernel@vger.kernel.org
>主题: Re: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting
>
>On Tue, Jan 09, 2018 at 11:30:09AM +0800, Chen Feng wrote:
>>
>>
>> On 2018/1/9 18:43, Zeng Tao wrote:
>> > This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
>> > Add cached pool to spead up cached buffer alloc"),
>
>Use the Fixes tag.
>
>Fixes: e7f63771b60e ("ION: Sys_heap: Add cached pool to spead up cached
>buffer alloc")
>
Agree, thanks. 

>> the gfp_mask low
>> > order pool is overlapped by the high order inside the loop, so the
>> > gfp_mask of all pools are set to high_order_gfp_flags.
>> >
>>
>> Thanks
>> > Signed-off-by: Zeng Tao <prime.zeng@hisilicon.com>
>> > ---
>> >  drivers/staging/android/ion/ion_system_heap.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/staging/android/ion/ion_system_heap.c
>> > b/drivers/staging/android/ion/ion_system_heap.c
>> > index 4dc5d7a..b6386be 100644
>> > --- a/drivers/staging/android/ion/ion_system_heap.c
>> > +++ b/drivers/staging/android/ion/ion_system_heap.c
>> > @@ -298,10 +298,10 @@ static int ion_system_heap_create_pools(struct
>ion_page_pool **pools,
>> >  					bool cached)
>> >  {
>> >  	int i;
>> > -	gfp_t gfp_flags = low_order_gfp_flags;
>> >
>> >  	for (i = 0; i < NUM_ORDERS; i++) {
>> >  		struct ion_page_pool *pool;
>> > +		gfp_t gfp_flags = low_order_gfp_flags;
>>
>> Not define here. Better "gfp_flags = low_order_gfp_flags"
>
>Either way is fine... 
>

>regards,
>dan carpenter

Re: 答复: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Laura Abbott]

On 01/09/2018 04:06 AM, Zengtao (B) wrote:
>> -----邮件原件-----
>> 发件人: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>> 发送时间: 2018年1月9日 17:14
>> 收件人: Chenfeng (puck) <puck.chen@hisilicon.com>
>> 抄送: Zengtao (B) <prime.zeng@hisilicon.com>; labbott@redhat.com;
>> sumit.semwal@linaro.org; gregkh@linuxfoundation.org; arve@android.com;
>> tkjos@android.com; maco@android.com; devel@driverdev.osuosl.org;
>> linux-kernel@vger.kernel.org
>> 主题: Re: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting
>>
>> On Tue, Jan 09, 2018 at 11:30:09AM +0800, Chen Feng wrote:
>>>
>>>
>>> On 2018/1/9 18:43, Zeng Tao wrote:
>>>> This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
>>>> Add cached pool to spead up cached buffer alloc"),
>>
>> Use the Fixes tag.
>>
>> Fixes: e7f63771b60e ("ION: Sys_heap: Add cached pool to spead up cached
>> buffer alloc")
>>
> Agree, thanks.
> 

If you're going to be fixing this, it would be good to
fix the other problems pointed out (stop with the
#define of the flags).

Thanks,
Laura

答复: 答复: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Zengtao (B)]

>-----邮件原件-----
>发件人: Laura Abbott [mailto:labbott@redhat.com]
>发送时间: 2018年1月11日 8:01
>收件人: Zengtao (B) <prime.zeng@hisilicon.com>; Dan Carpenter
><dan.carpenter@oracle.com>; Chenfeng (puck) <puck.chen@hisilicon.com>
>抄送: sumit.semwal@linaro.org; gregkh@linuxfoundation.org;
>arve@android.com; tkjos@android.com; maco@android.com;
>devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
>主题: Re: 答复: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask
>setting
>
>On 01/09/2018 04:06 AM, Zengtao (B) wrote:
>>> -----邮件原件-----
>>> 发件人: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>>> 发送时间: 2018年1月9日 17:14
>>> 收件人: Chenfeng (puck) <puck.chen@hisilicon.com>
>>> 抄送: Zengtao (B) <prime.zeng@hisilicon.com>; labbott@redhat.com;
>>> sumit.semwal@linaro.org; gregkh@linuxfoundation.org;
>>> arve@android.com; tkjos@android.com; maco@android.com;
>>> devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
>>> 主题: Re: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask
>>> setting
>>>
>>> On Tue, Jan 09, 2018 at 11:30:09AM +0800, Chen Feng wrote:
>>>>
>>>>
>>>> On 2018/1/9 18:43, Zeng Tao wrote:
>>>>> This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
>>>>> Add cached pool to spead up cached buffer alloc"),
>>>
>>> Use the Fixes tag.
>>>
>>> Fixes: e7f63771b60e ("ION: Sys_heap: Add cached pool to spead up
>>> cached buffer alloc")
>>>
>> Agree, thanks.
>>
>
>If you're going to be fixing this, it would be good to fix the other problems
>pointed out (stop with the #define of the flags).
>
It is OK, I will fix in the new version fix.  

And to make the code more explicit, I have to choices of fixes:
Choice 1: 
if (orders[i] > 4)
	gfp_flags = high_order_gfp_flags;
else
	gfp_flags = low_order_gfp_flags;
Choice 2:
gfp_flags = (orders[i] > 4) ? high_order_gfp_flags : low_order_gfp_flags;

Any suggestion ?


BTW, I found another problem related: 
Currently the order 4 and order 0 allocation flag haven't got the __GFP_NOWARN set, 
if the order 4 allocation failed but the allocation of order 0 success, it will print warning
message which is useless.

Of course, this is not related to this fix, but this is what I have met when test this fix.

Thanks
Zengtao 

Re: 答复: 答复: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask setting

[Laura Abbott]

On 01/10/2018 06:06 PM, Zengtao (B) wrote:
>> -----邮件原件-----
>> 发件人: Laura Abbott [mailto:labbott@redhat.com]
>> 发送时间: 2018年1月11日 8:01
>> 收件人: Zengtao (B) <prime.zeng@hisilicon.com>; Dan Carpenter
>> <dan.carpenter@oracle.com>; Chenfeng (puck) <puck.chen@hisilicon.com>
>> 抄送: sumit.semwal@linaro.org; gregkh@linuxfoundation.org;
>> arve@android.com; tkjos@android.com; maco@android.com;
>> devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
>> 主题: Re: 答复: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask
>> setting
>>
>> On 01/09/2018 04:06 AM, Zengtao (B) wrote:
>>>> -----邮件原件-----
>>>> 发件人: Dan Carpenter [mailto:dan.carpenter@oracle.com]
>>>> 发送时间: 2018年1月9日 17:14
>>>> 收件人: Chenfeng (puck) <puck.chen@hisilicon.com>
>>>> 抄送: Zengtao (B) <prime.zeng@hisilicon.com>; labbott@redhat.com;
>>>> sumit.semwal@linaro.org; gregkh@linuxfoundation.org;
>>>> arve@android.com; tkjos@android.com; maco@android.com;
>>>> devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
>>>> 主题: Re: [PATCH] ION: Sys_heap: fix the incorrect pool->gfp_mask
>>>> setting
>>>>
>>>> On Tue, Jan 09, 2018 at 11:30:09AM +0800, Chen Feng wrote:
>>>>>
>>>>>
>>>>> On 2018/1/9 18:43, Zeng Tao wrote:
>>>>>> This issue is introduced by the commit <e7f63771b60e> ("ION: Sys_heap:
>>>>>> Add cached pool to spead up cached buffer alloc"),
>>>>
>>>> Use the Fixes tag.
>>>>
>>>> Fixes: e7f63771b60e ("ION: Sys_heap: Add cached pool to spead up
>>>> cached buffer alloc")
>>>>
>>> Agree, thanks.
>>>
>>
>> If you're going to be fixing this, it would be good to fix the other problems
>> pointed out (stop with the #define of the flags).
>>
> It is OK, I will fix in the new version fix.
> 
> And to make the code more explicit, I have to choices of fixes:
> Choice 1:
> if (orders[i] > 4)
> 	gfp_flags = high_order_gfp_flags;
> else
> 	gfp_flags = low_order_gfp_flags;
> Choice 2:
> gfp_flags = (orders[i] > 4) ? high_order_gfp_flags : low_order_gfp_flags;
> 

I prefer #1, but please make sure you are following the
suggestions in https://marc.info/?l=linux-kernel&m=151518104214191&w=2

> Any suggestion ?
> 
> 
> BTW, I found another problem related:
> Currently the order 4 and order 0 allocation flag haven't got the __GFP_NOWARN set,
> if the order 4 allocation failed but the allocation of order 0 success, it will print warning
> message which is useless.
> 
> Of course, this is not related to this fix, but this is what I have met when test this fix.
> 

Yes, go ahead and fix that in a separate patch too.

> Thanks
> Zengtao
> 

Thanks,
Laura