| Message ID | 20210127201031.98544-4-alobakin@pm.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: consolidate page_is_pfmemalloc() usage | expand |
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>
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.
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 <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
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 <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
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,
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
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(-)