On 2024/7/3 0:00, Alexander Duyck wrote:

...

quoted

quoted

quoted

+
+    offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
+    if (unlikely(offset + fragsz > size)) {

The fragsz check below could be moved to here.

quoted

             page = virt_to_page(nc->va);

             if (!page_ref_sub_and_test(page, nc->pagecnt_bias))

@@ -99,17 +100,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
                     goto refill;
             }

-#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
-            /* if size can vary use size else just use PAGE_SIZE */
-            size = nc->size;
-#endif
             /* OK, page count is 0, we can safely set it */
             set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);

             /* reset page count bias and offset to start of new frag */
             nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
-            offset = size - fragsz;
-            if (unlikely(offset < 0)) {
+            offset = 0;
+            if (unlikely(fragsz > PAGE_SIZE)) {

Since we aren't taking advantage of the flag that is left after the
subtraction we might just want to look at moving this piece up to just
after the offset + fragsz check. That should prevent us from trying to
refill if we have a request that is larger than a single page. In
addition we could probably just drop the 3 PAGE_SIZE checks above as
they would be redundant.

I am not sure I understand the 'drop the 3 PAGE_SIZE checks' part and
the 'redundant' part, where is the '3 PAGE_SIZE checks'? And why they
are redundant?

I was referring to the addition of the checks for align > PAGE_SIZE in
the alloc functions at the start of this diff. I guess I had dropped
them from the first half of it with the "...". Also looking back
through the patch you misspelled "avoid" as "aovid".

The issue is there is a ton of pulling things forward that don't
necessarily make sense into these diffs. Now that I have finished
looking through the set I have a better idea of why those are there
and they might make sense. It is just difficult to review since code
is being added for things that aren't applicable to the patch being
reviewed.

As you mentioned in other thread, perhaps the 'remaining' changing does
need to be incorporated into this patch.

.