A bunch of drivers test the page before reusing/recycling for two
common conditions:
- if a page was allocated under memory pressure (pfmemalloc page);
- if a page was allocated at a distant memory node (to exclude
slowdowns).
Introduce and use a new common function for doing this and eliminate
all functions-duplicates from drivers.
Suggested-by: David Rientjes <[email protected]>
Signed-off-by: Alexander Lobakin <[email protected]>
---
drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 10 ++--------
drivers/net/ethernet/intel/fm10k/fm10k_main.c | 9 ++-------
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 15 +--------------
drivers/net/ethernet/intel/iavf/iavf_txrx.c | 15 +--------------
drivers/net/ethernet/intel/ice/ice_txrx.c | 11 +----------
drivers/net/ethernet/intel/igb/igb_main.c | 7 +------
drivers/net/ethernet/intel/igc/igc_main.c | 7 +------
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 +------
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 7 +------
drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 7 +------
include/linux/skbuff.h | 15 +++++++++++++++
11 files changed, 27 insertions(+), 83 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 512080640cbc..f71e3d963750 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -2800,12 +2800,6 @@ static void hns3_nic_alloc_rx_buffers(struct hns3_enet_ring *ring,
writel(i, ring->tqp->io_base + HNS3_RING_RX_RING_HEAD_REG);
}
-static bool hns3_page_is_reusable(struct page *page)
-{
- return page_to_nid(page) == numa_mem_id() &&
- !page_is_pfmemalloc(page);
-}
-
static bool hns3_can_reuse_page(struct hns3_desc_cb *cb)
{
return (page_count(cb->priv) - cb->pagecnt_bias) == 1;
@@ -2826,7 +2820,7 @@ static void hns3_nic_reuse_page(struct sk_buff *skb, int i,
/* Avoid re-using remote pages, or the stack is still using the page
* when page_offset rollback to zero, flag default unreuse
*/
- if (unlikely(!hns3_page_is_reusable(desc_cb->priv)) ||
+ if (unlikely(dev_page_is_reserved(desc_cb->priv)) ||
(!desc_cb->page_offset && !hns3_can_reuse_page(desc_cb))) {
__page_frag_cache_drain(desc_cb->priv, desc_cb->pagecnt_bias);
return;
@@ -3084,7 +3078,7 @@ static int hns3_alloc_skb(struct hns3_enet_ring *ring, unsigned int length,
memcpy(__skb_put(skb, length), va, ALIGN(length, sizeof(long)));
/* We can reuse buffer as-is, just make sure it is local */
- if (likely(hns3_page_is_reusable(desc_cb->priv)))
+ if (likely(!dev_page_is_reserved(desc_cb->priv)))
desc_cb->reuse_flag = 1;
else /* This page cannot be reused so discard it */
__page_frag_cache_drain(desc_cb->priv,
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_main.c b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
index 99b8252eb969..9f547dd8c914 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_main.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_main.c
@@ -194,17 +194,12 @@ static void fm10k_reuse_rx_page(struct fm10k_ring *rx_ring,
DMA_FROM_DEVICE);
}
-static inline bool fm10k_page_is_reserved(struct page *page)
-{
- return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
-}
-
static bool fm10k_can_reuse_rx_page(struct fm10k_rx_buffer *rx_buffer,
struct page *page,
unsigned int __maybe_unused truesize)
{
/* avoid re-using remote pages */
- if (unlikely(fm10k_page_is_reserved(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
@@ -266,7 +261,7 @@ static bool fm10k_add_rx_frag(struct fm10k_rx_buffer *rx_buffer,
memcpy(__skb_put(skb, size), va, ALIGN(size, sizeof(long)));
/* page is not reserved, we can reuse buffer as-is */
- if (likely(!fm10k_page_is_reserved(page)))
+ if (likely(!dev_page_is_reserved(page)))
return true;
/* this page cannot be reused so discard it */
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 2574e78f7597..4c295671aa09 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -1843,19 +1843,6 @@ static bool i40e_cleanup_headers(struct i40e_ring *rx_ring, struct sk_buff *skb,
return false;
}
-/**
- * i40e_page_is_reusable - check if any reuse is possible
- * @page: page struct to check
- *
- * A page is not reusable if it was allocated under low memory
- * conditions, or it's not in the same NUMA node as this CPU.
- */
-static inline bool i40e_page_is_reusable(struct page *page)
-{
- return (page_to_nid(page) == numa_mem_id()) &&
- !page_is_pfmemalloc(page);
-}
-
/**
* i40e_can_reuse_rx_page - Determine if this page can be reused by
* the adapter for another receive
@@ -1891,7 +1878,7 @@ static bool i40e_can_reuse_rx_page(struct i40e_rx_buffer *rx_buffer,
struct page *page = rx_buffer->page;
/* Is any reuse possible? */
- if (unlikely(!i40e_page_is_reusable(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index 256fa07d54d5..a6b552465f03 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -1141,19 +1141,6 @@ static void iavf_reuse_rx_page(struct iavf_ring *rx_ring,
new_buff->pagecnt_bias = old_buff->pagecnt_bias;
}
-/**
- * iavf_page_is_reusable - check if any reuse is possible
- * @page: page struct to check
- *
- * A page is not reusable if it was allocated under low memory
- * conditions, or it's not in the same NUMA node as this CPU.
- */
-static inline bool iavf_page_is_reusable(struct page *page)
-{
- return (page_to_nid(page) == numa_mem_id()) &&
- !page_is_pfmemalloc(page);
-}
-
/**
* iavf_can_reuse_rx_page - Determine if this page can be reused by
* the adapter for another receive
@@ -1187,7 +1174,7 @@ static bool iavf_can_reuse_rx_page(struct iavf_rx_buffer *rx_buffer)
struct page *page = rx_buffer->page;
/* Is any reuse possible? */
- if (unlikely(!iavf_page_is_reusable(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 422f53997c02..623bbd27870f 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -728,15 +728,6 @@ bool ice_alloc_rx_bufs(struct ice_ring *rx_ring, u16 cleaned_count)
return !!cleaned_count;
}
-/**
- * ice_page_is_reserved - check if reuse is possible
- * @page: page struct to check
- */
-static bool ice_page_is_reserved(struct page *page)
-{
- return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
-}
-
/**
* ice_rx_buf_adjust_pg_offset - Prepare Rx buffer for reuse
* @rx_buf: Rx buffer to adjust
@@ -776,7 +767,7 @@ ice_can_reuse_rx_page(struct ice_rx_buf *rx_buf, int rx_buf_pgcnt)
struct page *page = rx_buf->page;
/* avoid re-using remote pages */
- if (unlikely(ice_page_is_reserved(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 84d4284b8b32..f3020a73323d 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8215,18 +8215,13 @@ static void igb_reuse_rx_page(struct igb_ring *rx_ring,
new_buff->pagecnt_bias = old_buff->pagecnt_bias;
}
-static inline bool igb_page_is_reserved(struct page *page)
-{
- return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
-}
-
static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
{
unsigned int pagecnt_bias = rx_buffer->pagecnt_bias;
struct page *page = rx_buffer->page;
/* avoid re-using remote pages */
- if (unlikely(igb_page_is_reserved(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 43aec42e6d9d..8d51fb86be2e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1648,18 +1648,13 @@ static void igc_reuse_rx_page(struct igc_ring *rx_ring,
new_buff->pagecnt_bias = old_buff->pagecnt_bias;
}
-static inline bool igc_page_is_reserved(struct page *page)
-{
- return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
-}
-
static bool igc_can_reuse_rx_page(struct igc_rx_buffer *rx_buffer)
{
unsigned int pagecnt_bias = rx_buffer->pagecnt_bias;
struct page *page = rx_buffer->page;
/* avoid re-using remote pages */
- if (unlikely(igc_page_is_reserved(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e08c01525fd2..37cc117c3aa6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -1940,11 +1940,6 @@ static void ixgbe_reuse_rx_page(struct ixgbe_ring *rx_ring,
new_buff->pagecnt_bias = old_buff->pagecnt_bias;
}
-static inline bool ixgbe_page_is_reserved(struct page *page)
-{
- return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
-}
-
static bool ixgbe_can_reuse_rx_page(struct ixgbe_rx_buffer *rx_buffer,
int rx_buffer_pgcnt)
{
@@ -1952,7 +1947,7 @@ static bool ixgbe_can_reuse_rx_page(struct ixgbe_rx_buffer *rx_buffer,
struct page *page = rx_buffer->page;
/* avoid re-using remote pages */
- if (unlikely(ixgbe_page_is_reserved(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index a14e55e7fce8..24caa0d2d572 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -781,18 +781,13 @@ static void ixgbevf_reuse_rx_page(struct ixgbevf_ring *rx_ring,
new_buff->pagecnt_bias = old_buff->pagecnt_bias;
}
-static inline bool ixgbevf_page_is_reserved(struct page *page)
-{
- return (page_to_nid(page) != numa_mem_id()) || page_is_pfmemalloc(page);
-}
-
static bool ixgbevf_can_reuse_rx_page(struct ixgbevf_rx_buffer *rx_buffer)
{
unsigned int pagecnt_bias = rx_buffer->pagecnt_bias;
struct page *page = rx_buffer->page;
/* avoid re-using remote pages */
- if (unlikely(ixgbevf_page_is_reserved(page)))
+ if (unlikely(dev_page_is_reserved(page)))
return false;
#if (PAGE_SIZE < 8192)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index dec93d57542f..5b9fb979adc6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -212,11 +212,6 @@ static inline u32 mlx5e_decompress_cqes_start(struct mlx5e_rq *rq,
return mlx5e_decompress_cqes_cont(rq, wq, 1, budget_rem) - 1;
}
-static inline bool mlx5e_page_is_reserved(struct page *page)
-{
- return page_is_pfmemalloc(page) || page_to_nid(page) != numa_mem_id();
-}
-
static inline bool mlx5e_rx_cache_put(struct mlx5e_rq *rq,
struct mlx5e_dma_info *dma_info)
{
@@ -229,7 +224,7 @@ static inline bool mlx5e_rx_cache_put(struct mlx5e_rq *rq,
return false;
}
- if (unlikely(mlx5e_page_is_reserved(dma_info->page))) {
+ if (unlikely(dev_page_is_reserved(dma_info->page))) {
stats->cache_waive++;
return false;
}
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b027526da4f9..688782513d09 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2938,6 +2938,21 @@ static inline struct page *dev_alloc_page(void)
return dev_alloc_pages(0);
}
+/**
+ * dev_page_is_reserved - check whether a page can be reused for network Rx
+ * @page: the page to test
+ *
+ * A page shouldn't be considered for reusing/recycling if it was allocated
+ * under memory pressure or at a distant memory node.
+ *
+ * Returns true if this page should be returned to page allocator, false
+ * otherwise.
+ */
+static inline bool dev_page_is_reserved(const struct page *page)
+{
+ return page_is_pfmemalloc(page) || page_to_nid(page) != numa_mem_id();
+}
+
/**
* skb_propagate_pfmemalloc - Propagate pfmemalloc if skb is allocated after RX page
* @page: The page that was allocated from skb_alloc_page
--
2.30.0
Alexander Lobakin wrote:
> A bunch of drivers test the page before reusing/recycling for two
> common conditions:
> - if a page was allocated under memory pressure (pfmemalloc page);
> - if a page was allocated at a distant memory node (to exclude
> slowdowns).
>
> Introduce and use a new common function for doing this and eliminate
> all functions-duplicates from drivers.
>
> Suggested-by: David Rientjes <[email protected]>
> Signed-off-by: Alexander Lobakin <[email protected]>
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 10 ++--------
> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 9 ++-------
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 15 +--------------
> drivers/net/ethernet/intel/iavf/iavf_txrx.c | 15 +--------------
> drivers/net/ethernet/intel/ice/ice_txrx.c | 11 +----------
> drivers/net/ethernet/intel/igb/igb_main.c | 7 +------
> drivers/net/ethernet/intel/igc/igc_main.c | 7 +------
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 +------
> drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 7 +------
> drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 7 +------
> include/linux/skbuff.h | 15 +++++++++++++++
> 11 files changed, 27 insertions(+), 83 deletions(-)
For the patch, and esp. for the Intel drivers:
Reviewed-by: Jesse Brandeburg <[email protected]>
On Wed, 27 Jan 2021, Alexander Lobakin wrote:
> A bunch of drivers test the page before reusing/recycling for two
> common conditions:
> - if a page was allocated under memory pressure (pfmemalloc page);
> - if a page was allocated at a distant memory node (to exclude
> slowdowns).
>
> Introduce and use a new common function for doing this and eliminate
> all functions-duplicates from drivers.
>
> Suggested-by: David Rientjes <[email protected]>
> Signed-off-by: Alexander Lobakin <[email protected]>
Looks even better than I thought!
(Since all of the changes are in drivers/net/ethernet/, I assume
everything directly or indirectly includes skbuff.h already.)
Acked-by: David Rientjes <[email protected]>
Thanks for doing this.
On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote:
> + * dev_page_is_reserved - check whether a page can be reused for network Rx
> + * @page: the page to test
> + *
> + * A page shouldn't be considered for reusing/recycling if it was allocated
> + * under memory pressure or at a distant memory node.
> + *
> + * Returns true if this page should be returned to page allocator, false
> + * otherwise.
> + */
> +static inline bool dev_page_is_reserved(const struct page *page)
Am I the only one who feels like "reusable" is a better term than
"reserved".
From: Jakub Kicinski <[email protected]>
Date: Fri, 29 Jan 2021 18:39:07 -0800
> On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote:
> > + * dev_page_is_reserved - check whether a page can be reused for network Rx
> > + * @page: the page to test
> > + *
> > + * A page shouldn't be considered for reusing/recycling if it was allocated
> > + * under memory pressure or at a distant memory node.
> > + *
> > + * Returns true if this page should be returned to page allocator, false
> > + * otherwise.
> > + */
> > +static inline bool dev_page_is_reserved(const struct page *page)
>
> Am I the only one who feels like "reusable" is a better term than
> "reserved".
I thought about it, but this will need to inverse the conditions in
most of the drivers. I decided to keep it as it is.
I can redo if "reusable" is preferred.
Regarding "no objectives to take patch 1 through net-next": patches
2-3 depend on it, so I can't put it in a separate series.
Thanks,
Al
On Sat, 30 Jan 2021 15:42:29 +0000 Alexander Lobakin wrote:
> > On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote:
> > > + * dev_page_is_reserved - check whether a page can be reused for network Rx
> > > + * @page: the page to test
> > > + *
> > > + * A page shouldn't be considered for reusing/recycling if it was allocated
> > > + * under memory pressure or at a distant memory node.
> > > + *
> > > + * Returns true if this page should be returned to page allocator, false
> > > + * otherwise.
> > > + */
> > > +static inline bool dev_page_is_reserved(const struct page *page)
> >
> > Am I the only one who feels like "reusable" is a better term than
> > "reserved".
>
> I thought about it, but this will need to inverse the conditions in
> most of the drivers. I decided to keep it as it is.
> I can redo if "reusable" is preferred.
Naming is hard. As long as the condition is not a double negative it
reads fine to me, but that's probably personal preference.
The thing that doesn't sit well is the fact that there is nothing
"reserved" about a page from another NUMA node.. But again, if nobody
+1s this it's whatever...
That said can we move the likely()/unlikely() into the helper itself?
People on the internet may say otherwise but according to my tests
using __builtin_expect() on a return value of a static inline helper
works just fine.
From: Jakub Kicinski <[email protected]>
Date: Sat, 30 Jan 2021 11:07:07 -0800
> On Sat, 30 Jan 2021 15:42:29 +0000 Alexander Lobakin wrote:
> > > On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote:
> > > > + * dev_page_is_reserved - check whether a page can be reused for network Rx
> > > > + * @page: the page to test
> > > > + *
> > > > + * A page shouldn't be considered for reusing/recycling if it was allocated
> > > > + * under memory pressure or at a distant memory node.
> > > > + *
> > > > + * Returns true if this page should be returned to page allocator, false
> > > > + * otherwise.
> > > > + */
> > > > +static inline bool dev_page_is_reserved(const struct page *page)
> > >
> > > Am I the only one who feels like "reusable" is a better term than
> > > "reserved".
> >
> > I thought about it, but this will need to inverse the conditions in
> > most of the drivers. I decided to keep it as it is.
> > I can redo if "reusable" is preferred.
>
> Naming is hard. As long as the condition is not a double negative it
> reads fine to me, but that's probably personal preference.
> The thing that doesn't sit well is the fact that there is nothing
> "reserved" about a page from another NUMA node.. But again, if nobody
> +1s this it's whatever...
Agree on NUMA and naming. I'm a bit surprised that 95% of drivers
have this helper called "reserved" (one of the reasons why I finished
with this variant).
Let's say, if anybody else will vote for "reusable", I'll pick it for
v3.
> That said can we move the likely()/unlikely() into the helper itself?
> People on the internet may say otherwise but according to my tests
> using __builtin_expect() on a return value of a static inline helper
> works just fine.
Sounds fine, this will make code more elegant. Will publish v3 soon.
Thanks,
Al
On 1/30/21 11:45 AM, Alexander Lobakin wrote:
> From: Jakub Kicinski <[email protected]>
> Date: Sat, 30 Jan 2021 11:07:07 -0800
>
>> On Sat, 30 Jan 2021 15:42:29 +0000 Alexander Lobakin wrote:
>>>> On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote:
>>>>> + * dev_page_is_reserved - check whether a page can be reused for network Rx
>>>>> + * @page: the page to test
>>>>> + *
>>>>> + * A page shouldn't be considered for reusing/recycling if it was allocated
>>>>> + * under memory pressure or at a distant memory node.
>>>>> + *
>>>>> + * Returns true if this page should be returned to page allocator, false
>>>>> + * otherwise.
>>>>> + */
>>>>> +static inline bool dev_page_is_reserved(const struct page *page)
>>>>
>>>> Am I the only one who feels like "reusable" is a better term than
>>>> "reserved".
>>>
>>> I thought about it, but this will need to inverse the conditions in
>>> most of the drivers. I decided to keep it as it is.
>>> I can redo if "reusable" is preferred.
>>
>> Naming is hard. As long as the condition is not a double negative it
>> reads fine to me, but that's probably personal preference.
>> The thing that doesn't sit well is the fact that there is nothing
>> "reserved" about a page from another NUMA node.. But again, if nobody
>> +1s this it's whatever...
>
> Agree on NUMA and naming. I'm a bit surprised that 95% of drivers
> have this helper called "reserved" (one of the reasons why I finished
> with this variant).
> Let's say, if anybody else will vote for "reusable", I'll pick it for
> v3.
Definitely "reusable" seems better to me, and especially anything *other*
than "reserved" is a good idea, IMHO.
thanks,
--
John Hubbard
NVIDIA