On 2024/8/15 1:54, Alexander H Duyck wrote:

On Thu, 2024-08-08 at 20:37 +0800, Yunsheng Lin wrote:

quoted

Refactor common codes from __page_frag_alloc_va_align()
to __page_frag_cache_reload(), so that the new API can
make use of them.

CC: Alexander Duyck <redacted>
Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/page_frag_cache.h |   2 +-
 mm/page_frag_cache.c            | 138 ++++++++++++++++++--------------
 2 files changed, 81 insertions(+), 59 deletions(-)

diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
index 4ce924eaf1b1..0abffdd10a1c 100644
--- a/include/linux/page_frag_cache.h
+++ b/include/linux/page_frag_cache.h

@@ -52,7 +52,7 @@ static inline void *encoded_page_address(unsigned long encoded_va)
 
 static inline void page_frag_cache_init(struct page_frag_cache *nc)
 {
-	nc->encoded_va = 0;
+	memset(nc, 0, sizeof(*nc));
 }
 

Still not a fan of this. Just setting encoded_va to 0 should be enough
as the other fields will automatically be overwritten when the new page
is allocated.

Relying on memset is problematic at best since you then introduce the
potential for issues where remaining somehow gets corrupted but
encoded_va/page is 0. I would rather have both of these being checked
as a part of allocation than just just assuming it is valid if
remaining is set.

Does adding something like VM_BUG_ON(!nc->encoded_va && nc->remaining) to
catch the above problem address your above concern?

I would prefer to keep the check for a non-0 encoded_page value and
then check remaining rather than just rely on remaining as it creates a
single point of failure. With that we can safely tear away a page and
the next caller to try to allocate will populated a new page and the
associated fields.

As mentioned before, the memset() is used mainly because of:
1. avoid a checking in the fast path.
2. avoid duplicating the checking pattern you mentioned above for the
   new API.

quoted

 static inline bool page_frag_cache_is_pfmemalloc(struct page_frag_cache *nc)

diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
index 2544b292375a..4e6b1c4684f0 100644
--- a/mm/page_frag_cache.c
+++ b/mm/page_frag_cache.c

@@ -19,8 +19,27 @@
 #include <linux/page_frag_cache.h>
 #include "internal.h"
 

...

quoted

+
+/* Reload cache by reusing the old cache if it is possible, or
+ * refilling from the page allocator.
+ */
+static bool __page_frag_cache_reload(struct page_frag_cache *nc,
+				     gfp_t gfp_mask)
+{
+	if (likely(nc->encoded_va)) {
+		if (__page_frag_cache_reuse(nc->encoded_va, nc->pagecnt_bias))
+			goto out;
+	}
+
+	if (unlikely(!__page_frag_cache_refill(nc, gfp_mask)))
+		return false;
+
+out:
+	/* reset page count bias and remaining to start of new frag */
+	nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
+	nc->remaining = page_frag_cache_page_size(nc->encoded_va);

One thought I am having is that it might be better to have the
pagecnt_bias get set at the same time as the page_ref_add or the
set_page_count call. In addition setting the remaining value at the
same time probably would make sense as in the refill case you can make
use of the "order" value directly instead of having to write/read it
out of the encoded va/page.

Probably, there is always tradeoff to make regarding avoid code
duplication and avoid reading the order, I am not sure it matters
for both for case, I would rather keep the above pattern if there
is not obvious benefit for the other pattern.

With that we could simplify this function and get something closer to
what we had for the original alloc_va_align code.

quoted

+	return true;
 }
 
 void page_frag_cache_drain(struct page_frag_cache *nc)

@@ -55,7 +100,7 @@ void page_frag_cache_drain(struct page_frag_cache *nc)
 
 	__page_frag_cache_drain(virt_to_head_page((void *)nc->encoded_va),
 				nc->pagecnt_bias);
-	nc->encoded_va = 0;
+	memset(nc, 0, sizeof(*nc));
 }
 EXPORT_SYMBOL(page_frag_cache_drain);
 

@@ -73,67 +118,44 @@ void *__page_frag_alloc_va_align(struct page_frag_cache *nc,
 				 unsigned int align_mask)
 {
 	unsigned long encoded_va = nc->encoded_va;
-	unsigned int size, remaining;
-	struct page *page;
-
-	if (unlikely(!encoded_va)) {

We should still be checking this before we even touch remaining.
Otherwise we greatly increase the risk of providing a bad virtual
address and have greatly decreased the likelihood of us catching
potential errors gracefully.

quoted

-refill:
-		page = __page_frag_cache_refill(nc, gfp_mask);
-		if (!page)
-			return NULL;
-
-		encoded_va = nc->encoded_va;
-		size = page_frag_cache_page_size(encoded_va);
-
-		/* Even if we own the page, we do not use atomic_set().
-		 * This would break get_page_unless_zero() users.
-		 */
-		page_ref_add(page, PAGE_FRAG_CACHE_MAX_SIZE);
-
-		/* reset page count bias and remaining to start of new frag */
-		nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
-		nc->remaining = size;

With my suggested change above you could essentially just drop the
block starting from the comment and this function wouldn't need to
change as much as it is.

It seems you are still suggesting that new API also duplicates the old
checking pattern in __page_frag_alloc_va_align()?

I would rather avoid the above if something like VM_BUG_ON() can address
your above concern.