* Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer
2017-06-02 14:43 ` Michal Hocko
@ 2017-06-02 15:41 ` Tom Lendacky
2017-06-03 1:25 ` [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER " kbuild test robot
1 sibling, 0 replies; 5+ messages in thread
From: Tom Lendacky @ 2017-06-02 15:41 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm, LKML
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>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer
2017-06-02 14:43 ` Michal Hocko
2017-06-02 15:41 ` Tom Lendacky
@ 2017-06-03 1:25 ` kbuild test robot
1 sibling, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2017-06-03 1:25 UTC (permalink / raw)
To: Michal Hocko; +Cc: kbuild-all, Tom Lendacky, linux-mm, LKML
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread