strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Michal Hocko]

Hi Tom,
I have stumbled over the following construct in xgbe_map_rx_buffer
	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?

Thanks!
-- 
Michal Hocko
SUSE Labs

strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Michal Hocko]

Hi Tom,
I have stumbled over the following construct in xgbe_map_rx_buffer
	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?

Thanks!
-- 
Michal Hocko
SUSE Labs

--
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: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Tom Lendacky]

On 5/31/2017 11:04 AM, Michal Hocko wrote:
> Hi Tom,

Hi Michal,

> I have stumbled over the following construct in xgbe_map_rx_buffer
> 	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
> which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
> And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?
> 

The driver tries to allocate a number of pages to be used as receive
buffers.  Based on what I could find in documentation, the value of
PAGE_ALLOC_COSTLY_ORDER is the point at which order allocations
(could) get expensive.  So I decrease by one the order requested. The
max_t test is just to insure that in case PAGE_ALLOC_COSTLY_ORDER ever
gets defined as 0, 0 would be used.

I believe there have been some enhancements relative to speed in
allocating 0-order pages recently that may make this unnecessary. I
haven't run any performance tests yet to determine if I can just go to
a 0-order allocation, though.

Thanks,
Tom

> Thanks!
> 

Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Tom Lendacky]

On 5/31/2017 11:04 AM, Michal Hocko wrote:
> Hi Tom,

Hi Michal,

> I have stumbled over the following construct in xgbe_map_rx_buffer
> 	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
> which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
> And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?
> 

The driver tries to allocate a number of pages to be used as receive
buffers.  Based on what I could find in documentation, the value of
PAGE_ALLOC_COSTLY_ORDER is the point at which order allocations
(could) get expensive.  So I decrease by one the order requested. The
max_t test is just to insure that in case PAGE_ALLOC_COSTLY_ORDER ever
gets defined as 0, 0 would be used.

I believe there have been some enhancements relative to speed in
allocating 0-order pages recently that may make this unnecessary. I
haven't run any performance tests yet to determine if I can just go to
a 0-order allocation, though.

Thanks,
Tom

> Thanks!
> 

--
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: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Michal Hocko]

On Fri 02-06-17 09:20:54, Tom Lendacky wrote:
> On 5/31/2017 11:04 AM, Michal Hocko wrote:
> >Hi Tom,
> 
> Hi Michal,
> 
> >I have stumbled over the following construct in xgbe_map_rx_buffer
> >	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
> >which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
> >And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?
> >
> 
> The driver tries to allocate a number of pages to be used as receive
> buffers.  Based on what I could find in documentation, the value of
> PAGE_ALLOC_COSTLY_ORDER is the point at which order allocations
> (could) get expensive.  So I decrease by one the order requested. The
> max_t test is just to insure that in case PAGE_ALLOC_COSTLY_ORDER ever
> gets defined as 0, 0 would be used.

So you have fallen into a carefully prepared trap ;). The thing is that
orders _larger_ than PAGE_ALLOC_COSTLY_ORDER are costly actually. I can
completely see how this can be confusing.

Moreover xgbe_map_rx_buffer does an atomic allocation which doesn't do
any direct reclaim/compaction attempts so the costly vs. non-costly
doesn't apply here at all.

I would be much happier if no code outside of mm used
PAGE_ALLOC_COSTLY_ORDER directly but that requires a deeper
consideration. E.g. what would be the largest size that would be
useful for this path? xgbe_alloc_pages does the order fallback so
PAGE_ALLOC_COSTLY_ORDER sounds like an artificial limit to me.
I guess we can at least simplify the xgbe right away though.
---
>From c7d5ca637b889c4e3779f8d2a84ade6448a76ef9 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Fri, 2 Jun 2017 16:34:28 +0200
Subject: [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer

xgbe_map_rx_buffer is rather confused about what PAGE_ALLOC_COSTLY_ORDER
means. It uses PAGE_ALLOC_COSTLY_ORDER-1 assuming that
PAGE_ALLOC_COSTLY_ORDER is the first costly order which is not the case
actually because orders larger than that are costly. And even that
applies only to sleeping allocations which is not the case here. We
simply do not perform any costly operations like reclaim or compaction
for those. Simplify the code by dropping the order calculation and use
PAGE_ALLOC_COSTLY_ORDER directly.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-desc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
index b3bc87fe3764..5ded10eba418 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
@@ -333,9 +333,8 @@ static int xgbe_map_rx_buffer(struct xgbe_prv_data *pdata,
 	}
 
 	if (!ring->rx_buf_pa.pages) {
-		order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
 		ret = xgbe_alloc_pages(pdata, &ring->rx_buf_pa, GFP_ATOMIC,
-				       order);
+				       PAGE_ALLOC_COSTLY_ORDER);
 		if (ret)
 			return ret;
 	}
-- 
2.11.0


-- 
Michal Hocko
SUSE Labs

Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Michal Hocko]

On Fri 02-06-17 09:20:54, Tom Lendacky wrote:
> On 5/31/2017 11:04 AM, Michal Hocko wrote:
> >Hi Tom,
> 
> Hi Michal,
> 
> >I have stumbled over the following construct in xgbe_map_rx_buffer
> >	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
> >which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
> >And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?
> >
> 
> The driver tries to allocate a number of pages to be used as receive
> buffers.  Based on what I could find in documentation, the value of
> PAGE_ALLOC_COSTLY_ORDER is the point at which order allocations
> (could) get expensive.  So I decrease by one the order requested. The
> max_t test is just to insure that in case PAGE_ALLOC_COSTLY_ORDER ever
> gets defined as 0, 0 would be used.

So you have fallen into a carefully prepared trap ;). The thing is that
orders _larger_ than PAGE_ALLOC_COSTLY_ORDER are costly actually. I can
completely see how this can be confusing.

Moreover xgbe_map_rx_buffer does an atomic allocation which doesn't do
any direct reclaim/compaction attempts so the costly vs. non-costly
doesn't apply here at all.

I would be much happier if no code outside of mm used
PAGE_ALLOC_COSTLY_ORDER directly but that requires a deeper
consideration. E.g. what would be the largest size that would be
useful for this path? xgbe_alloc_pages does the order fallback so
PAGE_ALLOC_COSTLY_ORDER sounds like an artificial limit to me.
I guess we can at least simplify the xgbe right away though.
---

Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Tom Lendacky]

On 6/2/2017 9:43 AM, Michal Hocko wrote:
> On Fri 02-06-17 09:20:54, Tom Lendacky wrote:
>> On 5/31/2017 11:04 AM, Michal Hocko wrote:
>>> Hi Tom,
>>
>> Hi Michal,
>>
>>> I have stumbled over the following construct in xgbe_map_rx_buffer
>>> 	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
>>> which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
>>> And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?
>>>
>>
>> The driver tries to allocate a number of pages to be used as receive
>> buffers.  Based on what I could find in documentation, the value of
>> PAGE_ALLOC_COSTLY_ORDER is the point at which order allocations
>> (could) get expensive.  So I decrease by one the order requested. The
>> max_t test is just to insure that in case PAGE_ALLOC_COSTLY_ORDER ever
>> gets defined as 0, 0 would be used.
> 
> So you have fallen into a carefully prepared trap ;). The thing is that
> orders _larger_ than PAGE_ALLOC_COSTLY_ORDER are costly actually. I can
> completely see how this can be confusing.
> 
> Moreover xgbe_map_rx_buffer does an atomic allocation which doesn't do
> any direct reclaim/compaction attempts so the costly vs. non-costly
> doesn't apply here at all.
> 
> I would be much happier if no code outside of mm used
> PAGE_ALLOC_COSTLY_ORDER directly but that requires a deeper
> consideration. E.g. what would be the largest size that would be
> useful for this path? xgbe_alloc_pages does the order fallback so
> PAGE_ALLOC_COSTLY_ORDER sounds like an artificial limit to me.
> I guess we can at least simplify the xgbe right away though.
> ---
>  From c7d5ca637b889c4e3779f8d2a84ade6448a76ef9 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Fri, 2 Jun 2017 16:34:28 +0200
> Subject: [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer
> 
> xgbe_map_rx_buffer is rather confused about what PAGE_ALLOC_COSTLY_ORDER
> means. It uses PAGE_ALLOC_COSTLY_ORDER-1 assuming that
> PAGE_ALLOC_COSTLY_ORDER is the first costly order which is not the case
> actually because orders larger than that are costly. And even that
> applies only to sleeping allocations which is not the case here. We
> simply do not perform any costly operations like reclaim or compaction
> for those. Simplify the code by dropping the order calculation and use
> PAGE_ALLOC_COSTLY_ORDER directly.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>   drivers/net/ethernet/amd/xgbe/xgbe-desc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
> index b3bc87fe3764..5ded10eba418 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
> @@ -333,9 +333,8 @@ static int xgbe_map_rx_buffer(struct xgbe_prv_data *pdata,
>   	}
>   
>   	if (!ring->rx_buf_pa.pages) {
> -		order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
>   		ret = xgbe_alloc_pages(pdata, &ring->rx_buf_pa, GFP_ATOMIC,
> -				       order);
> +				       PAGE_ALLOC_COSTLY_ORDER);

You'll need to also remove the variable definition to avoid an un-used
variable warning.  You should also send this to the netdev mailing list
to send this through the net-next tree (or net tree if you want it fixed
in the current version of the Linux kernel).

Thanks,
Tom

>   		if (ret)
>   			return ret;
>   	}
> 

Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

[Tom Lendacky]

On 6/2/2017 9:43 AM, Michal Hocko wrote:
> On Fri 02-06-17 09:20:54, Tom Lendacky wrote:
>> On 5/31/2017 11:04 AM, Michal Hocko wrote:
>>> Hi Tom,
>>
>> Hi Michal,
>>
>>> I have stumbled over the following construct in xgbe_map_rx_buffer
>>> 	order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
>>> which looks quite suspicious. Why does it PAGE_ALLOC_COSTLY_ORDER - 1?
>>> And why do you depend on PAGE_ALLOC_COSTLY_ORDER at all?
>>>
>>
>> The driver tries to allocate a number of pages to be used as receive
>> buffers.  Based on what I could find in documentation, the value of
>> PAGE_ALLOC_COSTLY_ORDER is the point at which order allocations
>> (could) get expensive.  So I decrease by one the order requested. The
>> max_t test is just to insure that in case PAGE_ALLOC_COSTLY_ORDER ever
>> gets defined as 0, 0 would be used.
> 
> So you have fallen into a carefully prepared trap ;). The thing is that
> orders _larger_ than PAGE_ALLOC_COSTLY_ORDER are costly actually. I can
> completely see how this can be confusing.
> 
> Moreover xgbe_map_rx_buffer does an atomic allocation which doesn't do
> any direct reclaim/compaction attempts so the costly vs. non-costly
> doesn't apply here at all.
> 
> I would be much happier if no code outside of mm used
> PAGE_ALLOC_COSTLY_ORDER directly but that requires a deeper
> consideration. E.g. what would be the largest size that would be
> useful for this path? xgbe_alloc_pages does the order fallback so
> PAGE_ALLOC_COSTLY_ORDER sounds like an artificial limit to me.
> I guess we can at least simplify the xgbe right away though.
> ---
>  From c7d5ca637b889c4e3779f8d2a84ade6448a76ef9 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Fri, 2 Jun 2017 16:34:28 +0200
> Subject: [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer
> 
> xgbe_map_rx_buffer is rather confused about what PAGE_ALLOC_COSTLY_ORDER
> means. It uses PAGE_ALLOC_COSTLY_ORDER-1 assuming that
> PAGE_ALLOC_COSTLY_ORDER is the first costly order which is not the case
> actually because orders larger than that are costly. And even that
> applies only to sleeping allocations which is not the case here. We
> simply do not perform any costly operations like reclaim or compaction
> for those. Simplify the code by dropping the order calculation and use
> PAGE_ALLOC_COSTLY_ORDER directly.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>   drivers/net/ethernet/amd/xgbe/xgbe-desc.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
> index b3bc87fe3764..5ded10eba418 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
> @@ -333,9 +333,8 @@ static int xgbe_map_rx_buffer(struct xgbe_prv_data *pdata,
>   	}
>   
>   	if (!ring->rx_buf_pa.pages) {
> -		order = max_t(int, PAGE_ALLOC_COSTLY_ORDER - 1, 0);
>   		ret = xgbe_alloc_pages(pdata, &ring->rx_buf_pa, GFP_ATOMIC,
> -				       order);
> +				       PAGE_ALLOC_COSTLY_ORDER);

You'll need to also remove the variable definition to avoid an un-used
variable warning.  You should also send this to the netdev mailing list
to send this through the net-next tree (or net tree if you want it fixed
in the current version of the Linux kernel).

Thanks,
Tom

>   		if (ret)
>   			return ret;
>   	}
> 

--
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] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2885 bytes --]

Hi Michal,

[auto build test WARNING on net-next/master]
[also build test WARNING on v4.12-rc3 next-20170602]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/amd-xgbe-use-PAGE_ALLOC_COSTLY_ORDER-in-xgbe_map_rx_buffer/20170603-021038
config: x86_64-randconfig-v0-06030836 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/net//ethernet/amd/xgbe/xgbe-desc.c: In function 'xgbe_map_rx_buffer':
>> drivers/net//ethernet/amd/xgbe/xgbe-desc.c:327: warning: unused variable 'order'

vim +/order +327 drivers/net//ethernet/amd/xgbe/xgbe-desc.c

174fd259 Lendacky, Thomas 2014-11-04  311  	if ((pa->pages_offset + len) > pa->pages_len) {
08dcc47c Lendacky, Thomas 2014-11-04  312  		/* This data descriptor is responsible for unmapping page(s) */
174fd259 Lendacky, Thomas 2014-11-04  313  		bd->pa_unmap = *pa;
08dcc47c Lendacky, Thomas 2014-11-04  314  
08dcc47c Lendacky, Thomas 2014-11-04  315  		/* Get a new allocation next time */
174fd259 Lendacky, Thomas 2014-11-04  316  		pa->pages = NULL;
174fd259 Lendacky, Thomas 2014-11-04  317  		pa->pages_len = 0;
174fd259 Lendacky, Thomas 2014-11-04  318  		pa->pages_offset = 0;
174fd259 Lendacky, Thomas 2014-11-04  319  		pa->pages_dma = 0;
174fd259 Lendacky, Thomas 2014-11-04  320  	}
174fd259 Lendacky, Thomas 2014-11-04  321  }
174fd259 Lendacky, Thomas 2014-11-04  322  
174fd259 Lendacky, Thomas 2014-11-04  323  static int xgbe_map_rx_buffer(struct xgbe_prv_data *pdata,
174fd259 Lendacky, Thomas 2014-11-04  324  			      struct xgbe_ring *ring,
174fd259 Lendacky, Thomas 2014-11-04  325  			      struct xgbe_ring_data *rdata)
174fd259 Lendacky, Thomas 2014-11-04  326  {
174fd259 Lendacky, Thomas 2014-11-04 @327  	int order, ret;
174fd259 Lendacky, Thomas 2014-11-04  328  
174fd259 Lendacky, Thomas 2014-11-04  329  	if (!ring->rx_hdr_pa.pages) {
174fd259 Lendacky, Thomas 2014-11-04  330  		ret = xgbe_alloc_pages(pdata, &ring->rx_hdr_pa, GFP_ATOMIC, 0);
174fd259 Lendacky, Thomas 2014-11-04  331  		if (ret)
174fd259 Lendacky, Thomas 2014-11-04  332  			return ret;
08dcc47c Lendacky, Thomas 2014-11-04  333  	}
08dcc47c Lendacky, Thomas 2014-11-04  334  
174fd259 Lendacky, Thomas 2014-11-04  335  	if (!ring->rx_buf_pa.pages) {

:::::: The code at line 327 was first introduced by commit
:::::: 174fd2597b0bd8c19fce6a97e8b0f753ef4ce7cb amd-xgbe: Implement split header receive support

:::::: TO: Lendacky, Thomas <Thomas.Lendacky@amd.com>
:::::: CC: David S. Miller <davem@davemloft.net>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28663 bytes --]