On Tue, Jul 2, 2024 at 5:28 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2024/7/2 7:27, Alexander H Duyck wrote:
> > On Tue, 2024-06-25 at 21:52 +0800, Yunsheng Lin wrote:
> >> We are above to use page_frag_alloc_*() API to not just
> > "about to use", not "above to use"
>
> Ack.
>
> >
> >> allocate memory for skb->data, but also use them to do
> >> the memory allocation for skb frag too. Currently the
> >> implementation of page_frag in mm subsystem is running
> >> the offset as a countdown rather than count-up value,
> >> there may have several advantages to that as mentioned
> >> in [1], but it may have some disadvantages, for example,
> >> it may disable skb frag coaleasing and more correct cache
> >> prefetching
> >>
>
> ...
>
> >> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> >> index 88f567ef0e29..da244851b8a4 100644
> >> --- a/mm/page_frag_cache.c
> >> +++ b/mm/page_frag_cache.c
> >> @@ -72,10 +72,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >> if (!page)
> >> return NULL;
> >>
> >> -#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> >> - /* if size can vary use size else just use PAGE_SIZE */
> >> - size = nc->size;
> >> -#endif
> >> /* Even if we own the page, we do not use atomic_set().
> >> * This would break get_page_unless_zero() users.
> >> */
> >> @@ -84,11 +80,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >> /* reset page count bias and offset to start of new frag */
> >> nc->pfmemalloc = page_is_pfmemalloc(page);
> >> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> >> - nc->offset = size;
> >> + nc->offset = 0;
> >> }
> >>
> >> - offset = nc->offset - fragsz;
> >> - if (unlikely(offset < 0)) {
> >> +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> >> + /* if size can vary use size else just use PAGE_SIZE */
> >> + size = nc->size;
> >> +#endif
> >> +
> >> + offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
> >> + if (unlikely(offset + fragsz > size)) {
> >
> > The fragsz check below could be moved to here.
> >
> >> 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.
