Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1654389 > unrolled thread

strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

Back to article view | Back to linux.kernel

Contents

  strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer Michal Hocko <mhocko@kernel.org> - 2017-05-31 18:10 +0200
    Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer Michal Hocko <mhocko@kernel.org> - 2017-06-02 16:50 +0200

#1654389 — strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer

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

[toc] | [next] | [standalone]

#1656308

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web