2021-01-31 12:58:40

by Alexander Lobakin

[permalink] [raw]
Subject: [PATCH v3 net-next 3/5] net: introduce common dev_page_is_reusable()

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 a new common inline for doing this, with likely() already
folded inside to make driver code a bit simpler.

Suggested-by: David Rientjes <[email protected]>
Suggested-by: Jakub Kicinski <[email protected]>
Cc: John Hubbard <[email protected]>
Signed-off-by: Alexander Lobakin <[email protected]>
Reviewed-by: Jesse Brandeburg <[email protected]>
Acked-by: David Rientjes <[email protected]>
---
include/linux/skbuff.h | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b027526da4f9..0e42c53b8ca9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2938,6 +2938,22 @@ static inline struct page *dev_alloc_page(void)
return dev_alloc_pages(0);
}

+/**
+ * dev_page_is_reusable - 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 false if this page should be returned to page allocator, true
+ * otherwise.
+ */
+static inline bool dev_page_is_reusable(const struct page *page)
+{
+ return likely(page_to_nid(page) == numa_mem_id() &&
+ !page_is_pfmemalloc(page));
+}
+
/**
* 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



2021-01-31 15:33:33

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH v3 net-next 3/5] net: introduce common dev_page_is_reusable()

On Sun, Jan 31, 2021 at 12:11:52PM +0000, 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 a new common inline for doing this, with likely() already
> folded inside to make driver code a bit simpler.

I don't see the need for the 'dev_' prefix. That actually confuses me
because it makes me think this is tied to ZONE_DEVICE or some such.

So how about calling it just 'page_is_reusable' and putting it in mm.h
with page_is_pfmemalloc() and making the comment a little less network-centric?

Or call it something like skb_page_is_recyclable() since it's only used
by networking today. But I bet it could/should be used more widely.

> +/**
> + * dev_page_is_reusable - 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 false if this page should be returned to page allocator, true
> + * otherwise.
> + */
> +static inline bool dev_page_is_reusable(const struct page *page)
> +{
> + return likely(page_to_nid(page) == numa_mem_id() &&
> + !page_is_pfmemalloc(page));
> +}
> +

2021-01-31 15:38:58

by Alexander Lobakin

[permalink] [raw]
Subject: Re: [PATCH v3 net-next 3/5] net: introduce common dev_page_is_reusable()

From: Matthew Wilcox <[email protected]>
Date: Sun, 31 Jan 2021 12:22:05 +0000

> On Sun, Jan 31, 2021 at 12:11:52PM +0000, 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 a new common inline for doing this, with likely() already
> > folded inside to make driver code a bit simpler.
>
> I don't see the need for the 'dev_' prefix. That actually confuses me
> because it makes me think this is tied to ZONE_DEVICE or some such.

Several functions right above this one also use 'dev_' prefix. It's
a rather old mark that it's about network devices.

> So how about calling it just 'page_is_reusable' and putting it in mm.h
> with page_is_pfmemalloc() and making the comment a little less network-centric?

This pair of conditions (!pfmemalloc + local memory node) is really
specific to network drivers. I didn't see any other instances of such
tests, so I don't see a reason to place it in a more common mm.h.

> Or call it something like skb_page_is_recyclable() since it's only used
> by networking today. But I bet it could/should be used more widely.

There's nothing about skb. Tested page is just a memory chunk for DMA
transaction. It can be used as skb head/frag, for XDP buffer/frame or
for XSK umem.

> > +/**
> > + * dev_page_is_reusable - 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 false if this page should be returned to page allocator, true
> > + * otherwise.
> > + */
> > +static inline bool dev_page_is_reusable(const struct page *page)
> > +{
> > + return likely(page_to_nid(page) == numa_mem_id() &&
> > + !page_is_pfmemalloc(page));
> > +}
> > +

Al