Re: [PATCH net-next v2 2/3] page_pool: support non-frag page for page_pool_alloc_frag()

[Alexander Duyck]

On Wed, May 31, 2023 at 5:19 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2023/5/30 23:07, Alexander H Duyck wrote:
> ...
>
> >> +    if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> >> +            *offset = 0;
> >> +            return page_pool_alloc_pages(pool, gfp);
> >> +    }
> >> +
> >
> > This is a recipe for pain. Rather than doing this I would say we should
> > stick with our existing behavior and not allow page pool fragments to
> > be used when the DMA address is consuming the region. Otherwise we are
> > going to make things very confusing.
>
> Are there any other concern other than confusing? we could add a
> big comment to make it clear.
>
> The point of adding that is to avoid the driver handling the
> PAGE_POOL_DMA_USE_PP_FRAG_COUNT when using page_pool_alloc_frag()
> like something like below:
>
> if (!PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
>         page = page_pool_alloc_frag()
> else
>         page = XXXXX;
>
> Or do you perfer the driver handling it? why?
>
> >
> > If we have to have both version I would much rather just have some
> > inline calls in the header wrapped in one #ifdef for
> > PAGE_POOL_DMA_USE_PP_FRAG_COUNT that basically are a wrapper for
> > page_pool pages treated as pp_frag.
>
> Do you have a good name in mind for that wrapper.
> In addition to the naming, which API should I use when I am a driver
> author wanting to add page pool support?

When I usually have to deal with these sort of things I just rename
the original with a leading underscore or two and then just name the
inline the same as the original function.

> >
> >>      size = ALIGN(size, dma_get_cache_alignment());
> >> -    *offset = pool->frag_offset;
> >>
> >
> > If we are going to be allocating mono-frag pages they should be
> > allocated here based on the size check. That way we aren't discrupting
> > the performance for the smaller fragments and the code below could
> > function undisturbed.
>
> It is to allow possible optimization as below.

What optimization? From what I can tell you are taking extra steps for
non-page pool pages.

> >
> >> -    if (page && *offset + size > max_size) {
> >> +    if (page) {
> >> +            *offset = pool->frag_offset;
> >> +
> >> +            if (*offset + size <= max_size) {
> >> +                    pool->frag_users++;
> >> +                    pool->frag_offset = *offset + size;
> >> +                    alloc_stat_inc(pool, fast);
> >> +                    return page;
>
> Note that we still allow frag page here when '(size << 1 > max_size)'.

You are creating what I call a mono-frag. I am not a huge fan.

> >> +            }
> >> +
> >> +            pool->frag_page = NULL;
> >>              page = page_pool_drain_frag(pool, page);
> >>              if (page) {
> >>                      alloc_stat_inc(pool, fast);
> >> @@ -714,26 +727,24 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
> >>              }
> >>      }
> >>
> >> -    if (!page) {
> >> -            page = page_pool_alloc_pages(pool, gfp);
> >> -            if (unlikely(!page)) {
> >> -                    pool->frag_page = NULL;
> >> -                    return NULL;
> >> -            }
> >> -
> >> -            pool->frag_page = page;
> >> +    page = page_pool_alloc_pages(pool, gfp);
> >> +    if (unlikely(!page))
> >> +            return NULL;
> >>
> >>  frag_reset:
> >> -            pool->frag_users = 1;
> >> +    /* return page as non-frag page if a page is not able to
> >> +     * hold two frags for the current requested size.
> >> +     */
> >
> > This statement ins't exactly true since you make all page pool pages
> > into fragmented pages.
>
> Any suggestion to describe it more accurately?
> I wrote that thinking frag_count being one as non-frag page.

I wouldn't consider that to be the case. The problem is if frag count
== 1 then you have a fragmented page. It is no different from a page
where you had either freed earlier instances.

> >
> >
> >> +    if (unlikely(size << 1 > max_size)) {
> >
> > This should happen much sooner so you aren't mixing these allocations
> > with the smaller ones and forcing the fragmented page to be evicted.
>
> As mentioned above, it is to allow a possible optimization

Maybe you should point out exactly what you think the optimization is.
I don't see it as such. If you are going to evict anything that has a
size that is over half your max_size then you might as well just skip
using this entirely and just output a non-fragmented/mono frag page
rather than evicting the previously fragmented page.