linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage
@ 2021-01-27 20:10 Alexander Lobakin
  2021-01-27 20:11 ` [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument Alexander Lobakin
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-27 20:10 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: David Rientjes, Yisen Zhuang, Salil Mehta, Jesse Brandeburg,
	Tony Nguyen, Saeed Mahameed, Leon Romanovsky, Andrew Morton,
	Jesper Dangaard Brouer, Ilias Apalodimas, Jonathan Lemon,
	Willem de Bruijn, Randy Dunlap, Pablo Neira Ayuso, Dexuan Cui,
	Jakub Sitnicki, Marco Elver, Paolo Abeni, Alexander Lobakin,
	netdev, linux-kernel, intel-wired-lan, linux-rdma, linux-mm

page_is_pfmemalloc() is used mostly by networking drivers to test
if a page can be considered for reusing/recycling.
It doesn't write anything to the struct page itself, so its sole
argument can be constified, as well as the first argument of
skb_propagate_pfmemalloc().
In Page Pool core code, it can be simply inlined instead.
Most of the callers from NIC drivers were just doppelgangers of
the same condition tests. Derive them into a new common function
do deduplicate the code.

Since v1 [0]:
 - new: reduce code duplication by introducing a new common function
   to test if a page can be reused/recycled (David Rientjes);
 - collect autographs for Page Pool bits (Jesper Dangaard Brouer,
   Ilias Apalodimas).

[0] https://lore.kernel.org/netdev/20210125164612.243838-1-alobakin@pm.me

Alexander Lobakin (4):
  mm: constify page_is_pfmemalloc() argument
  skbuff: constify skb_propagate_pfmemalloc() "page" argument
  net: introduce common dev_page_is_reserved()
  net: page_pool: simplify page recycling condition tests

 .../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 +------
 .../net/ethernet/intel/ixgbevf/ixgbevf_main.c |  7 +------
 .../net/ethernet/mellanox/mlx5/core/en_rx.c   |  7 +------
 include/linux/mm.h                            |  2 +-
 include/linux/skbuff.h                        | 19 +++++++++++++++++--
 net/core/page_pool.c                          | 14 ++++----------
 13 files changed, 34 insertions(+), 96 deletions(-)

-- 
2.30.0




^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument
  2021-01-27 20:10 [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Alexander Lobakin
@ 2021-01-27 20:11 ` Alexander Lobakin
  2021-01-28 22:44   ` David Rientjes
  2021-01-30  2:30   ` Jakub Kicinski
  2021-01-27 20:11 ` [PATCH v2 net-next 2/4] skbuff: constify skb_propagate_pfmemalloc() "page" argument Alexander Lobakin
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-27 20:11 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: David Rientjes, Yisen Zhuang, Salil Mehta, Jesse Brandeburg,
	Tony Nguyen, Saeed Mahameed, Leon Romanovsky, Andrew Morton,
	Jesper Dangaard Brouer, Ilias Apalodimas, Jonathan Lemon,
	Willem de Bruijn, Randy Dunlap, Pablo Neira Ayuso, Dexuan Cui,
	Jakub Sitnicki, Marco Elver, Paolo Abeni, Alexander Lobakin,
	netdev, linux-kernel, intel-wired-lan, linux-rdma, linux-mm

The function only tests for page->index, so its argument should be
const.

Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 include/linux/mm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ecdf8a8cd6ae..078633d43af9 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1584,7 +1584,7 @@ struct address_space *page_mapping_file(struct page *page);
  * ALLOC_NO_WATERMARKS and the low watermark was not
  * met implying that the system is under some pressure.
  */
-static inline bool page_is_pfmemalloc(struct page *page)
+static inline bool page_is_pfmemalloc(const struct page *page)
 {
 	/*
 	 * Page index cannot be this large so this must be
-- 
2.30.0




^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 net-next 2/4] skbuff: constify skb_propagate_pfmemalloc() "page" argument
  2021-01-27 20:10 [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Alexander Lobakin
  2021-01-27 20:11 ` [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument Alexander Lobakin
@ 2021-01-27 20:11 ` Alexander Lobakin
  2021-01-28 22:45   ` David Rientjes
  2021-01-27 20:11 ` [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved() Alexander Lobakin
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-27 20:11 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: David Rientjes, Yisen Zhuang, Salil Mehta, Jesse Brandeburg,
	Tony Nguyen, Saeed Mahameed, Leon Romanovsky, Andrew Morton,
	Jesper Dangaard Brouer, Ilias Apalodimas, Jonathan Lemon,
	Willem de Bruijn, Randy Dunlap, Pablo Neira Ayuso, Dexuan Cui,
	Jakub Sitnicki, Marco Elver, Paolo Abeni, Alexander Lobakin,
	netdev, linux-kernel, intel-wired-lan, linux-rdma, linux-mm

The function doesn't write anything to the page struct itself,
so this argument can be const.

Misc: align second argument to the brace while at it.

Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 include/linux/skbuff.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9313b5aaf45b..b027526da4f9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2943,8 +2943,8 @@ static inline struct page *dev_alloc_page(void)
  *	@page: The page that was allocated from skb_alloc_page
  *	@skb: The skb that may need pfmemalloc set
  */
-static inline void skb_propagate_pfmemalloc(struct page *page,
-					     struct sk_buff *skb)
+static inline void skb_propagate_pfmemalloc(const struct page *page,
+					    struct sk_buff *skb)
 {
 	if (page_is_pfmemalloc(page))
 		skb->pfmemalloc = true;
-- 
2.30.0




^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-27 20:10 [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Alexander Lobakin
  2021-01-27 20:11 ` [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument Alexander Lobakin
  2021-01-27 20:11 ` [PATCH v2 net-next 2/4] skbuff: constify skb_propagate_pfmemalloc() "page" argument Alexander Lobakin
@ 2021-01-27 20:11 ` Alexander Lobakin
  2021-01-27 21:47   ` Jesse Brandeburg
                     ` (2 more replies)
  2021-01-27 20:11 ` [PATCH v2 net-next 4/4] net: page_pool: simplify page recycling condition tests Alexander Lobakin
  2021-01-27 21:52 ` [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Jesse Brandeburg
  4 siblings, 3 replies; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-27 20:11 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: David Rientjes, Yisen Zhuang, Salil Mehta, Jesse Brandeburg,
	Tony Nguyen, Saeed Mahameed, Leon Romanovsky, Andrew Morton,
	Jesper Dangaard Brouer, Ilias Apalodimas, Jonathan Lemon,
	Willem de Bruijn, Randy Dunlap, Pablo Neira Ayuso, Dexuan Cui,
	Jakub Sitnicki, Marco Elver, Paolo Abeni, Alexander Lobakin,
	netdev, linux-kernel, intel-wired-lan, linux-rdma, linux-mm

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 <rientjes@google.com>
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 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




^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 net-next 4/4] net: page_pool: simplify page recycling condition tests
  2021-01-27 20:10 [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Alexander Lobakin
                   ` (2 preceding siblings ...)
  2021-01-27 20:11 ` [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved() Alexander Lobakin
@ 2021-01-27 20:11 ` Alexander Lobakin
  2021-01-28 22:49   ` David Rientjes
  2021-01-27 21:52 ` [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Jesse Brandeburg
  4 siblings, 1 reply; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-27 20:11 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: David Rientjes, Yisen Zhuang, Salil Mehta, Jesse Brandeburg,
	Tony Nguyen, Saeed Mahameed, Leon Romanovsky, Andrew Morton,
	Jesper Dangaard Brouer, Ilias Apalodimas, Jonathan Lemon,
	Willem de Bruijn, Randy Dunlap, Pablo Neira Ayuso, Dexuan Cui,
	Jakub Sitnicki, Marco Elver, Paolo Abeni, Alexander Lobakin,
	netdev, linux-kernel, intel-wired-lan, linux-rdma, linux-mm

pool_page_reusable() is a leftover from pre-NUMA-aware times. For now,
this function is just a redundant wrapper over page_is_pfmemalloc(),
so Inline it into its sole call site.

Signed-off-by: Alexander Lobakin <alobakin@pm.me>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 net/core/page_pool.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f3c690b8c8e3..ad8b0707af04 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -350,14 +350,6 @@ static bool page_pool_recycle_in_cache(struct page *page,
 	return true;
 }
 
-/* page is NOT reusable when:
- * 1) allocated when system is under some pressure. (page_is_pfmemalloc)
- */
-static bool pool_page_reusable(struct page_pool *pool, struct page *page)
-{
-	return !page_is_pfmemalloc(page);
-}
-
 /* If the page refcnt == 1, this will try to recycle the page.
  * if PP_FLAG_DMA_SYNC_DEV is set, we'll try to sync the DMA area for
  * the configured size min(dma_sync_size, pool->max_len).
@@ -373,9 +365,11 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
 	 * regular page allocator APIs.
 	 *
 	 * refcnt == 1 means page_pool owns page, and can recycle it.
+	 *
+	 * page is NOT reusable when allocated when system is under
+	 * some pressure. (page_is_pfmemalloc)
 	 */
-	if (likely(page_ref_count(page) == 1 &&
-		   pool_page_reusable(pool, page))) {
+	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
 		/* Read barrier done in page_ref_count / READ_ONCE */
 
 		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
-- 
2.30.0




^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-27 20:11 ` [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved() Alexander Lobakin
@ 2021-01-27 21:47   ` Jesse Brandeburg
  2021-01-28 22:48   ` David Rientjes
  2021-01-30  2:39   ` Jakub Kicinski
  2 siblings, 0 replies; 17+ messages in thread
From: Jesse Brandeburg @ 2021-01-27 21:47 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, Jakub Kicinski, David Rientjes, Yisen Zhuang,
	Salil Mehta, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

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 <rientjes@google.com>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
>  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 <jesse.brandeburg@intel.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage
  2021-01-27 20:10 [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Alexander Lobakin
                   ` (3 preceding siblings ...)
  2021-01-27 20:11 ` [PATCH v2 net-next 4/4] net: page_pool: simplify page recycling condition tests Alexander Lobakin
@ 2021-01-27 21:52 ` Jesse Brandeburg
  4 siblings, 0 replies; 17+ messages in thread
From: Jesse Brandeburg @ 2021-01-27 21:52 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, Jakub Kicinski, David Rientjes, Yisen Zhuang,
	Salil Mehta, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

Alexander Lobakin wrote:

> page_is_pfmemalloc() is used mostly by networking drivers to test
> if a page can be considered for reusing/recycling.
> It doesn't write anything to the struct page itself, so its sole
> argument can be constified, as well as the first argument of
> skb_propagate_pfmemalloc().
> In Page Pool core code, it can be simply inlined instead.
> Most of the callers from NIC drivers were just doppelgangers of
> the same condition tests. Derive them into a new common function
> do deduplicate the code.

This is a useful cleanup! Thanks.

For the series:
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument
  2021-01-27 20:11 ` [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument Alexander Lobakin
@ 2021-01-28 22:44   ` David Rientjes
  2021-01-30  2:30   ` Jakub Kicinski
  1 sibling, 0 replies; 17+ messages in thread
From: David Rientjes @ 2021-01-28 22:44 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, Jakub Kicinski, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

On Wed, 27 Jan 2021, Alexander Lobakin wrote:

> The function only tests for page->index, so its argument should be
> const.
> 
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>

Acked-by: David Rientjes <rientjes@google.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 2/4] skbuff: constify skb_propagate_pfmemalloc() "page" argument
  2021-01-27 20:11 ` [PATCH v2 net-next 2/4] skbuff: constify skb_propagate_pfmemalloc() "page" argument Alexander Lobakin
@ 2021-01-28 22:45   ` David Rientjes
  0 siblings, 0 replies; 17+ messages in thread
From: David Rientjes @ 2021-01-28 22:45 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, Jakub Kicinski, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

On Wed, 27 Jan 2021, Alexander Lobakin wrote:

> The function doesn't write anything to the page struct itself,
> so this argument can be const.
> 
> Misc: align second argument to the brace while at it.
> 
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>

Acked-by: David Rientjes <rientjes@google.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-27 20:11 ` [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved() Alexander Lobakin
  2021-01-27 21:47   ` Jesse Brandeburg
@ 2021-01-28 22:48   ` David Rientjes
  2021-01-30  2:39   ` Jakub Kicinski
  2 siblings, 0 replies; 17+ messages in thread
From: David Rientjes @ 2021-01-28 22:48 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, Jakub Kicinski, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

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 <rientjes@google.com>
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>

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 <rientjes@google.com>

Thanks for doing this.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 4/4] net: page_pool: simplify page recycling condition tests
  2021-01-27 20:11 ` [PATCH v2 net-next 4/4] net: page_pool: simplify page recycling condition tests Alexander Lobakin
@ 2021-01-28 22:49   ` David Rientjes
  0 siblings, 0 replies; 17+ messages in thread
From: David Rientjes @ 2021-01-28 22:49 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, Jakub Kicinski, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

On Wed, 27 Jan 2021, Alexander Lobakin wrote:

> pool_page_reusable() is a leftover from pre-NUMA-aware times. For now,
> this function is just a redundant wrapper over page_is_pfmemalloc(),
> so Inline it into its sole call site.
> 
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Acked-by: David Rientjes <rientjes@google.com>


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument
  2021-01-27 20:11 ` [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument Alexander Lobakin
  2021-01-28 22:44   ` David Rientjes
@ 2021-01-30  2:30   ` Jakub Kicinski
  1 sibling, 0 replies; 17+ messages in thread
From: Jakub Kicinski @ 2021-01-30  2:30 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Alexander Lobakin, David S. Miller, David Rientjes, Yisen Zhuang,
	Salil Mehta, Jesse Brandeburg, Tony Nguyen, Saeed Mahameed,
	Leon Romanovsky, Andrew Morton, Jesper Dangaard Brouer,
	Ilias Apalodimas, Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

On Wed, 27 Jan 2021 20:11:01 +0000 Alexander Lobakin wrote:
> The function only tests for page->index, so its argument should be
> const.
> 
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
>  include/linux/mm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index ecdf8a8cd6ae..078633d43af9 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1584,7 +1584,7 @@ struct address_space *page_mapping_file(struct page *page);
>   * ALLOC_NO_WATERMARKS and the low watermark was not
>   * met implying that the system is under some pressure.
>   */
> -static inline bool page_is_pfmemalloc(struct page *page)
> +static inline bool page_is_pfmemalloc(const struct page *page)
>  {
>  	/*
>  	 * Page index cannot be this large so this must be

No objections for this going via net-next?


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-27 20:11 ` [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved() Alexander Lobakin
  2021-01-27 21:47   ` Jesse Brandeburg
  2021-01-28 22:48   ` David Rientjes
@ 2021-01-30  2:39   ` Jakub Kicinski
  2021-01-30 15:42     ` Alexander Lobakin
  2 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2021-01-30  2:39 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, David Rientjes, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

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".


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-30  2:39   ` Jakub Kicinski
@ 2021-01-30 15:42     ` Alexander Lobakin
  2021-01-30 19:07       ` Jakub Kicinski
  0 siblings, 1 reply; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-30 15:42 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Alexander Lobakin, David S. Miller, David Rientjes, Yisen Zhuang,
	Salil Mehta, Jesse Brandeburg, Tony Nguyen, Saeed Mahameed,
	Leon Romanovsky, Andrew Morton, Jesper Dangaard Brouer,
	Ilias Apalodimas, Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

From: Jakub Kicinski <kuba@kernel.org>
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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-30 15:42     ` Alexander Lobakin
@ 2021-01-30 19:07       ` Jakub Kicinski
  2021-01-30 19:45         ` Alexander Lobakin
  0 siblings, 1 reply; 17+ messages in thread
From: Jakub Kicinski @ 2021-01-30 19:07 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: David S. Miller, David Rientjes, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

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.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-30 19:07       ` Jakub Kicinski
@ 2021-01-30 19:45         ` Alexander Lobakin
  2021-01-30 21:23           ` John Hubbard
  0 siblings, 1 reply; 17+ messages in thread
From: Alexander Lobakin @ 2021-01-30 19:45 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Alexander Lobakin, David S. Miller, David Rientjes, Yisen Zhuang,
	Salil Mehta, Jesse Brandeburg, Tony Nguyen, Saeed Mahameed,
	Leon Romanovsky, Andrew Morton, Jesper Dangaard Brouer,
	Ilias Apalodimas, Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm

From: Jakub Kicinski <kuba@kernel.org>
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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved()
  2021-01-30 19:45         ` Alexander Lobakin
@ 2021-01-30 21:23           ` John Hubbard
  0 siblings, 0 replies; 17+ messages in thread
From: John Hubbard @ 2021-01-30 21:23 UTC (permalink / raw)
  To: Alexander Lobakin, Jakub Kicinski
  Cc: David S. Miller, David Rientjes, Yisen Zhuang, Salil Mehta,
	Jesse Brandeburg, Tony Nguyen, Saeed Mahameed, Leon Romanovsky,
	Andrew Morton, Jesper Dangaard Brouer, Ilias Apalodimas,
	Jonathan Lemon, Willem de Bruijn, Randy Dunlap,
	Pablo Neira Ayuso, Dexuan Cui, Jakub Sitnicki, Marco Elver,
	Paolo Abeni, netdev, linux-kernel, intel-wired-lan, linux-rdma,
	linux-mm


On 1/30/21 11:45 AM, Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 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


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-01-30 21:23 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 20:10 [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Alexander Lobakin
2021-01-27 20:11 ` [PATCH v2 net-next 1/4] mm: constify page_is_pfmemalloc() argument Alexander Lobakin
2021-01-28 22:44   ` David Rientjes
2021-01-30  2:30   ` Jakub Kicinski
2021-01-27 20:11 ` [PATCH v2 net-next 2/4] skbuff: constify skb_propagate_pfmemalloc() "page" argument Alexander Lobakin
2021-01-28 22:45   ` David Rientjes
2021-01-27 20:11 ` [PATCH v2 net-next 3/4] net: introduce common dev_page_is_reserved() Alexander Lobakin
2021-01-27 21:47   ` Jesse Brandeburg
2021-01-28 22:48   ` David Rientjes
2021-01-30  2:39   ` Jakub Kicinski
2021-01-30 15:42     ` Alexander Lobakin
2021-01-30 19:07       ` Jakub Kicinski
2021-01-30 19:45         ` Alexander Lobakin
2021-01-30 21:23           ` John Hubbard
2021-01-27 20:11 ` [PATCH v2 net-next 4/4] net: page_pool: simplify page recycling condition tests Alexander Lobakin
2021-01-28 22:49   ` David Rientjes
2021-01-27 21:52 ` [PATCH v2 net-next 0/4] net: consolidate page_is_pfmemalloc() usage Jesse Brandeburg

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).