Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230529092840.40413-1-linyunsheng@huawei.com>
 <20230529092840.40413-3-linyunsheng@huawei.com> <977d55210bfcb4f454b9d740fcbe6c451079a086.camel@gmail.com>
 <2e4f0359-151a-5cff-6d31-0ea0f014ef9a@huawei.com>
In-Reply-To: <2e4f0359-151a-5cff-6d31-0ea0f014ef9a@huawei.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Thu, 1 Jun 2023 11:14:45 -0700
Message-ID: <CAKgT0UcGYXstFP_H8VQtUooYEaYgDpG_crkodYOEyX4q0D58LQ@mail.gmail.com>
Subject: Re: [PATCH net-next v2 2/3] page_pool: support non-frag page for page_pool_alloc_frag()
To:     Yunsheng Lin <linyunsheng@huawei.com>
Cc:     davem@davemloft.net, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        Ilias Apalodimas <ilias.apalodimas@linaro.org>,
        Eric Dumazet <edumazet@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 31, 2023 at 5:19=E2=80=AFAM Yunsheng Lin <linyunsheng@huawei.co=
m> wrote:
>
> On 2023/5/30 23:07, Alexander H Duyck wrote:
> ...
>
> >> +    if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> >> +            *offset =3D 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 =3D page_pool_alloc_frag()
> else
>         page =3D 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 =3D ALIGN(size, dma_get_cache_alignment());
> >> -    *offset =3D 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 =3D pool->frag_offset;
> >> +
> >> +            if (*offset + size <=3D max_size) {
> >> +                    pool->frag_users++;
> >> +                    pool->frag_offset =3D *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 =3D NULL;
> >>              page =3D 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_po=
ol *pool,
> >>              }
> >>      }
> >>
> >> -    if (!page) {
> >> -            page =3D page_pool_alloc_pages(pool, gfp);
> >> -            if (unlikely(!page)) {
> >> -                    pool->frag_page =3D NULL;
> >> -                    return NULL;
> >> -            }
> >> -
> >> -            pool->frag_page =3D page;
> >> +    page =3D page_pool_alloc_pages(pool, gfp);
> >> +    if (unlikely(!page))
> >> +            return NULL;
> >>
> >>  frag_reset:
> >> -            pool->frag_users =3D 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
=3D=3D 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.