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: <20230609131740.7496-1-linyunsheng@huawei.com> <20230609131740.7496-4-linyunsheng@huawei.com>
 <CAKgT0UfVwQ=ri7ZDNnsATH2RQpEz+zDBBb6YprvniMEWGdw+dQ@mail.gmail.com> <36366741-8df2-1137-0dd9-d498d0f770e4@huawei.com>
In-Reply-To: <36366741-8df2-1137-0dd9-d498d0f770e4@huawei.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Wed, 14 Jun 2023 07:18:31 -0700
Message-ID: <CAKgT0UdXTSv1fDHBX4UC6Ok9NXKMJ_9F88CEv5TK+mpzy0N21g@mail.gmail.com>
Subject: Re: [PATCH net-next v3 3/4] page_pool: introduce page_pool_alloc() API
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 Tue, Jun 13, 2023 at 8:51=E2=80=AFPM Yunsheng Lin <linyunsheng@huawei.co=
m> wrote:
>
> On 2023/6/13 22:36, Alexander Duyck wrote:
> > On Fri, Jun 9, 2023 at 6:20=E2=80=AFAM Yunsheng Lin <linyunsheng@huawei=
.com> wrote:
>
> ...
>
> >>
> >> +static inline struct page *page_pool_alloc(struct page_pool *pool,
> >> +                                          unsigned int *offset,
> >> +                                          unsigned int *size, gfp_t g=
fp)
> >> +{
> >> +       unsigned int max_size =3D PAGE_SIZE << pool->p.order;
> >> +       struct page *page;
> >> +
> >> +       *size =3D ALIGN(*size, dma_get_cache_alignment());
> >> +
> >> +       if (WARN_ON(*size > max_size))
> >> +               return NULL;
> >> +
> >> +       if ((*size << 1) > max_size || PAGE_POOL_DMA_USE_PP_FRAG_COUNT=
) {
> >> +               *size =3D max_size;
> >> +               *offset =3D 0;
> >> +               return page_pool_alloc_pages(pool, gfp);
> >> +       }
> >> +
> >> +       page =3D __page_pool_alloc_frag(pool, offset, *size, gfp);
> >> +       if (unlikely(!page))
> >> +               return NULL;
> >> +
> >> +       /* There is very likely not enough space for another frag, so =
append the
> >> +        * remaining size to the current frag to avoid truesize undere=
stimate
> >> +        * problem.
> >> +        */
> >> +       if (pool->frag_offset + *size > max_size) {
> >> +               *size =3D max_size - *offset;
> >> +               pool->frag_offset =3D max_size;
> >> +       }
> >> +
> >
> > Rather than preventing a truesize underestimation this will cause one.
> > You are adding memory to the size of the page reserved and not
> > accounting for it anywhere as this isn't reported up to the network
> > stack. I would suggest dropping this from your patch.
>
> I was thinking about the driver author reporting it up to the network
> stack using the new API as something like below:
>
> int truesize =3D size;
> struct page *page;
> int offset;
>
> page =3D page_pool_dev_alloc(pool, &offset, &truesize);
> if (unlikely(!page))
>         goto err;
>
> skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>                 offset, size, truesize);
>
> and similiar handling for *_build_skb() case too.
>
> Does it make senses for that? or did I miss something obvious here?

It is more the fact that you are creating a solution in search of a
problem. As I said before most of the drivers will deal with these
sorts of issues by just doing the fragmentation themselves or
allocating fixed size frags and knowing how it will be divided into
the page.

If you are going to go down this path then you should have a consumer
for the API and fully implement it instead of taking half measures and
making truesize underreporting worse by evicting pages earlier.