net: Don't return pfmemalloc pages to the page pool.

PATCH In review Jonathan Lemon 3 participants 8 messages Dec 19, 2018 – Dec 21, 2018

Subsystems (3)

Networking Network drivers Page pool

lore

Share & tools

lore

Jonathan Lemon

author

2018-12-19 20:07 UTC #0 lore raw

PATCH 1/1 net: Don't return pfmemalloc pages to the page pool.

Return pfmemalloc pages back to the page allocator, instead of holding them in the page pool. While here, also use the __page_pool_return_page() API.

Signed-off-by Jonathan Lemon <[email protected]>

---
 net/core/page_pool.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

net/core/page_pool.c +2 -3

index 43a932cb609b..091007ff14a3 100644
@@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool *pool,
 	 *
 	 * refcnt == 1 means page_pool owns page, and can recycle it.
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {
 		/* Read barrier done in page_ref_count / READ_ONCE */
 
 		if (allow_direct && in_serving_softirq())

@@ -259,8 +259,7 @@ void __page_pool_put_page(struct page_pool *pool,
 	 * doing refcnt based recycle tricks, meaning another process
 	 * will be invoking put_page.
 	 */
-	__page_pool_clean_page(pool, page);
-	put_page(page);
+	__page_pool_return_page(pool, page);
 }
 EXPORT_SYMBOL(__page_pool_put_page);

Signature · 1 line

2.17.1

Jesper Dangaard Brouer

2018-12-20 13:03 UTC #1 lore raw

Quoted 2 lines · On Wed, 19 Dec 2018 12:06:51 -0800 Jonathan Lemon <[email protected]>…

Return pfmemalloc pages back to the page allocator, instead of holding them in the page pool.

Have you experience this issue in practice or is it theory?

Quoted 1 line

While here, also use the __page_pool_return_page() API.

Don't combine several unrelated changed in one patch.

Quoted 10 lines

Signed-off-by Jonathan Lemon <[email protected]>
---
 net/core/page_pool.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
net/core/page_pool.c +0 -0
index 43a932cb609b..091007ff14a3 100644

net/core/page_pool.c void __page_pool_put_page(struct page_pool *pool, · line 236

Quoted 5 lines

net/core/page_pool.c +1 -1

@@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool *pool,
 	 *
 	 * refcnt == 1 means page_pool owns page, and can recycle it.
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) {

I don't like adding this in the hot-path. Instead we could move this to the page alloc slow-path, and reject allocating pages with pgmemalloc in the first place.

Quoted 13 lines

net/core/page_pool.c +1 -2

 		/* Read barrier done in page_ref_count / READ_ONCE */
 
 		if (allow_direct && in_serving_softirq())

@@ -259,8 +259,7 @@ void __page_pool_put_page(struct page_pool *pool,
 	 * doing refcnt based recycle tricks, meaning another process
 	 * will be invoking put_page.
 	 */
-	__page_pool_clean_page(pool, page);
-	put_page(page);
+	__page_pool_return_page(pool, page);
 }
 EXPORT_SYMBOL(__page_pool_put_page);

Signature · 4 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Jonathan Lemon

author

2018-12-20 22:11 UTC #2 lore raw

(Resending due to mailer issues)

Quoted 7 lines · Jesper Dangaard Brouer

Quoted 2 lines · On Wed, 19 Dec 2018 12:06:51 -0800 Jonathan Lemon <[email protected]>…
Return pfmemalloc pages back to the page allocator, instead of holding them in the page pool.
Have you experience this issue in practice or is it theory?

We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and then return them back to the page allocator. (it's triggering the mlx5e_page_is_reserved() test). The page pool code isn't in production use, but the code paths appear identical.

Quoted 4 lines

Quoted 2 lines
While here, also use the __page_pool_return_page() API.
Don't combine several unrelated changed in one patch.

Okay - will send as 2 separate patches

Quoted 7 lines

net/core/page_pool.c +0 -0

index 43a932cb609b..091007ff14a3 100644
@@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool *pool,
 	 *
 	 * refcnt == 1 means page_pool owns page, and can recycle it.

Quoted 8 lines

net/core/page_pool.c void __page_pool_put_page(struct page_pool *pool,
Quoted 4 lines
net/core/page_pool.c +1 -1
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 && !page_is_pfmemalloc(page))) 
 {
I don't like adding this in the hot-path. Instead we could move this to the page alloc slow-path, and reject allocating pages with pgmemalloc in the first place.

No real objection to that - but then why bother with pfmemalloc? If the driver can't obtain pages for emergency use, then they might as well not exist.

Saeed Mahameed

2018-12-20 23:42 UTC #3 lore raw

Quoted 22 lines · Jonathan Lemon

(Resending due to mailer issues)
Quoted 7 lines · Jesper Dangaard Brouer
Quoted 2 lines · On Wed, 19 Dec 2018 12:06:51 -0800 Jonathan Lemon <[email protected]>…
Return pfmemalloc pages back to the page allocator, instead of holding them in the page pool.
Have you experience this issue in practice or is it theory?
We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and then
return them
back to the page allocator. (it's triggering the mlx5e_page_is_reserved() test).
The page pool code isn't in production use, but the code paths appear
identical.

Quoted 21 lines

Quoted 3 lines
Quoted 1 line
While here, also use the __page_pool_return_page() API.
Don't combine several unrelated changed in one patch.
Okay - will send as 2 separate patches
Quoted 14 lines
Quoted 13 lines
net/core/page_pool.c +1 -1
index 43a932cb609b..091007ff14a3 100644
@@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool
 *pool,
 	 *
 	 * refcnt == 1 means page_pool owns page, and can recycle it.
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 &&
 !page_is_pfmemalloc(page))) 
 {

I think this is wrong, if refcount is 1, then this page belongs to pagepool and you must enter this statement's true block, and test page_is_pfmemalloc inside (mark it unlikely),
to return a pfmemalloc page, you need to call __page_pool_return_page() to dma_unmap and other cleanups if required.

Quoted 5 lines

Quoted 4 lines
I don't like adding this in the hot-path. Instead we could move this
to the page alloc slow-path, and reject allocating pages with pgmemalloc in the first place.

I prefer to enhance the above solution, no need to risk pagepool users going out of memory under emergencies.

Quoted 8 lines

No real objection to that - but then why bother with pfmemalloc? If the
driver can't
obtain pages for emergency use, then they might as well not exist.
--
Jonathan

Jonathan Lemon

author

2018-12-21 00:56 UTC #4 lore raw

Quoted 45 lines · Saeed Mahameed

Quoted 22 lines · Jonathan Lemon
(Resending due to mailer issues)
Quoted 7 lines · Jesper Dangaard Brouer
Quoted 2 lines · On Wed, 19 Dec 2018 12:06:51 -0800 Jonathan Lemon <[email protected]>…
Return pfmemalloc pages back to the page allocator, instead of holding them in the page pool.
Have you experience this issue in practice or is it theory?
We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and then
return them
back to the page allocator. (it's triggering the mlx5e_page_is_reserved() test).
The page pool code isn't in production use, but the code paths appear
identical.
Quoted 21 lines
Quoted 3 lines
Quoted 1 line
While here, also use the __page_pool_return_page() API.
Don't combine several unrelated changed in one patch.
Okay - will send as 2 separate patches
Quoted 14 lines
Quoted 13 lines
net/core/page_pool.c +1 -1
index 43a932cb609b..091007ff14a3 100644
@@ -233,7 +233,7 @@ void __page_pool_put_page(struct page_pool
 *pool,
 	 *
 	 * refcnt == 1 means page_pool owns page, and can recycle it.
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 &&
 !page_is_pfmemalloc(page)))
 {

Quoted 5 lines

I think this is wrong, if refcount is 1, then this page belongs to pagepool and you must enter this statement's true block, and test page_is_pfmemalloc inside (mark it unlikely),
to return a pfmemalloc page, you need to call __page_pool_return_page() to dma_unmap and other cleanups if required.

If the page belongs to the pool, but it isn't recyclable, the fallback path is "__page_pool_return_page()". If it doesn't belong to the pool, it goes to the non-XDP mode, which is also __page_pool_return_page().

Does your dislike stem from the fact that the "non-XDP" mode is taken for the "refcount=1, pfmemalloc=T" case?

Signature · 1 line

Jonathan

Saeed Mahameed

2018-12-21 02:13 UTC #5 lore raw

Quoted 56 lines · Jonathan Lemon

Quoted 45 lines · Saeed Mahameed
Quoted 44 lines · Jonathan Lemon
(Resending due to mailer issues)
Quoted 8 lines · Jesper Dangaard Brouer
Quoted 3 lines · On Wed, 19 Dec 2018 12:06:51 -0800 Jonathan Lemon <[email protected]>…
Return pfmemalloc pages back to the page allocator, instead of
holding them in the page pool.
Have you experience this issue in practice or is it theory?
We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and then
return them
back to the page allocator. (it's triggering the mlx5e_page_is_reserved() test).
The page pool code isn't in production use, but the code paths appear
identical.
Quoted 3 lines
Quoted 1 line
While here, also use the __page_pool_return_page() API.
Don't combine several unrelated changed in one patch.
Okay - will send as 2 separate patches
Quoted 14 lines
Quoted 14 lines
net/core/page_pool.c +0 -0
index 43a932cb609b..091007ff14a3 100644
@@ -233,7 +233,7 @@ void __page_pool_put_page(struct
page_pool
*pool,
net/core/page_pool.c +0 -0
 	 *
 	 * refcnt == 1 means page_pool owns page, and can
recycle it.
net/core/page_pool.c +1 -1
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 &&
!page_is_pfmemalloc(page)))
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
{
I think this is wrong, if refcount is 1, then this page belongs to pagepool and you must enter this statement's true block, and test page_is_pfmemalloc inside (mark it unlikely),
to return a pfmemalloc page, you need to call __page_pool_return_page()
to dma_unmap and other cleanups if required.

Quoted 6 lines

If the page belongs to the pool, but it isn't recyclable, the fallback
path is "__page_pool_return_page()". If it doesn't belong to the pool,
it goes to the non-XDP mode, which is also __page_pool_return_page().

non-XDP mode doesn't explicitly call __page_pool_return_page() but it does exactly what __page_pool_return_page() is doing now, which is:

	__page_pool_clean_page(pool, page);
	put_page(page);

your code is logically correct for now, but semantically speaking __page_pool_return_page might change in the future to have special handling of pages that actually belong only to the pagepool (refcount==1), so better be safe and handle such pages in the pagepool path and not in the non-XDP path

all you need is:

    if(unlikely(!page_is_pfmemalloc(page))
           __page_pool_return_page()

early inside if (likely(page_ref_count(page) == 1)) pagepool recycling block; I don't think this will affect performance.

Quoted 2 lines

Does your dislike stem from the fact that the "non-XDP" mode is taken for the "refcount=1, pfmemalloc=T" case?

Saeed Mahameed

2018-12-21 02:26 UTC #6 lore raw

Quoted 79 lines · Saeed Mahameed

Quoted 58 lines · Jonathan Lemon
Quoted 48 lines · Saeed Mahameed
Quoted 31 lines · Jonathan Lemon
(Resending due to mailer issues)
Quoted 8 lines · Jesper Dangaard Brouer
Quoted 3 lines · On Wed, 19 Dec 2018 12:06:51 -0800 Jonathan Lemon <[email protected]>…
Return pfmemalloc pages back to the page allocator, instead of
holding them in the page pool.
Have you experience this issue in practice or is it theory?
We're seeing the mlx5 driver use pfmemalloc pages with 4.11, and
then
return them
back to the page allocator. (it's triggering the mlx5e_page_is_reserved() test).
The page pool code isn't in production use, but the code paths appear
identical.
Quoted 3 lines
Quoted 1 line
While here, also use the __page_pool_return_page() API.
Don't combine several unrelated changed in one patch.
Okay - will send as 2 separate patches
Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
net/core/page_pool.c +0 -0
index 43a932cb609b..091007ff14a3 100644
@@ -233,7 +233,7 @@ void __page_pool_put_page(struct
page_pool
*pool,
Quoted 8 lines
Quoted 8 lines
net/core/page_pool.c +0 -0
 	 *
 	 * refcnt == 1 means page_pool owns page, and can
recycle it.
net/core/page_pool.c +1 -1
 	 */
-	if (likely(page_ref_count(page) == 1)) {
+	if (likely(page_ref_count(page) == 1 &&
!page_is_pfmemalloc(page)))
{
Quoted 7 lines
I think this is wrong, if refcount is 1, then this page belongs to
pagepool and you must enter this statement's true block, and test page_is_pfmemalloc inside (mark it unlikely),
to return a pfmemalloc page, you need to call __page_pool_return_page()
to dma_unmap and other cleanups if required.
Quoted 7 lines
If the page belongs to the pool, but it isn't recyclable, the fallback
path is "__page_pool_return_page()". If it doesn't belong to the pool,
it goes to the non-XDP mode, which is also __page_pool_return_page().
non-XDP mode doesn't explicitly call __page_pool_return_page() but it does exactly what __page_pool_return_page() is doing now, which is:
	__page_pool_clean_page(pool, page);
	put_page(page);
your code is logically correct for now, but semantically speaking __page_pool_return_page might change in the future to have special handling of pages that actually belong only to the pagepool (refcount==1), so better be safe and handle such pages in the pagepool
path and not in the non-XDP path

Quoted 4 lines

all you need is:

    if(unlikely(!page_is_pfmemalloc(page))
           __page_pool_return_page()

Maybe something like:

@@ -236,6 +236,12 @@ void __page_pool_put_page(struct page_pool *pool,

        if (likely(page_ref_count(page) == 1)) {
                /* Read barrier done in page_ref_count / READ_ONCE */

+ /* Don't recycle emergency pages */
+ if(unlikely(page_is_pfmemalloc(page)) {
+ __page_pool_return_page(pool, page)
+ return;
+ }
+

** Not tested :) **

Quoted 7 lines

early inside if (likely(page_ref_count(page) == 1)) pagepool recycling
block; I don't think this will affect performance.
Quoted 3 lines
Does your dislike stem from the fact that the "non-XDP" mode is taken
for the "refcount=1, pfmemalloc=T" case?

Jesper Dangaard Brouer

2018-12-21 14:42 UTC #7 lore raw

In reply to Jonathan Lemon

Quoted 2 lines · On Thu, 20 Dec 2018 14:11:35 -0800 "Jonathan Lemon" <[email protected]>…

On 20 Dec 2018, at 5:03, Jesper Dangaard Brouer wrote:

[...]

Quoted 7 lines

Quoted 3 lines
I don't like adding this in the hot-path. Instead we could move this to the page alloc slow-path, and reject allocating pages with pgmemalloc in the first place.
No real objection to that - but then why bother with pfmemalloc? If the driver can't obtain pages for emergency use, then they might as well not exist.

I've changed my mind. There is an interesting opportunity in allowing pfmemalloc-pages to be used by the driver. (So, I'm saying I'm okay with adding this to the hot-path. And this hopefully doesn't affect performance (too much), as page_is_pfmemalloc() is reading from the same cache-line).

The opportunity is that XDP can handle/operate at wirespeed. We could allow XDP to get this info (simply via helper call, so we don't affect users not using this). When seeing PFMEMALLOC, which indicate a bad situation is occurring really soon, then we can react at a earlier stage (spending less cycles on reacting).
One idea is to reduce-size of XDP frame, and use XDP_TX to send-back
the frame to the sender as a congestion/drop notification, which inform sender to slowdown. If this is incast happening within the same data-center then the XDP_TX-feedback can reach the sender really fast. One example of such an approach: https://youtu.be/BO0QhaxBRr0

This is one example of how XDP can allow us to do stuff that was not possible before...

Signature · 4 lines

Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer