linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer
@ 2017-06-02 15:54 Michal Hocko
  2017-06-02 20:52 ` Tom Lendacky
  2017-06-05  0:01 ` David Miller
  0 siblings, 2 replies; 4+ messages in thread
From: Michal Hocko @ 2017-06-02 15:54 UTC (permalink / raw)
  To: Tom Lendacky; +Cc: netdev, linux-kernel, Michal Hocko

From: Michal Hocko <mhocko@suse.com>

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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
index b3bc87fe3764..0a98c369df20 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-desc.c
@@ -324,7 +324,7 @@ static int xgbe_map_rx_buffer(struct xgbe_prv_data *pdata,
 			      struct xgbe_ring *ring,
 			      struct xgbe_ring_data *rdata)
 {
-	int order, ret;
+	int ret;
 
 	if (!ring->rx_hdr_pa.pages) {
 		ret = xgbe_alloc_pages(pdata, &ring->rx_hdr_pa, GFP_ATOMIC, 0);
@@ -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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: strange PAGE_ALLOC_COSTLY_ORDER usage in xgbe_map_rx_buffer
@ 2017-06-02 14:43 Michal Hocko
  2017-06-03  1:25 ` [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER " kbuild test robot
  0 siblings, 1 reply; 4+ messages in thread
From: Michal Hocko @ 2017-06-02 14:43 UTC (permalink / raw)
  To: Tom Lendacky; +Cc: linux-mm, LKML

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-05  0:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-02 15:54 [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER in xgbe_map_rx_buffer Michal Hocko
2017-06-02 20:52 ` Tom Lendacky
2017-06-05  0:01 ` David Miller
  -- strict thread matches above, loose matches on Subject: below --
2017-06-02 14:43 strange PAGE_ALLOC_COSTLY_ORDER usage " Michal Hocko
2017-06-03  1:25 ` [PATCH] amd-xgbe: use PAGE_ALLOC_COSTLY_ORDER " kbuild test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).