Re: [PATCH net-next v3 3/4] page_pool: introduce page_pool_alloc() API

Yunsheng Lin
2023-06-14T03:51:59Z

On 2023/6/13 22:36, Alexander Duyck wrote:
> On Fri, Jun 9, 2023 at 6:20 AM 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 gfp)
>> +{
>> + unsigned int max_size = PAGE_SIZE << pool->p.order;
>> + struct page *page;
>> +
>> + *size = 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 = max_size;
>> + *offset = 0;
>> + return page_pool_alloc_pages(pool, gfp);
>> + }
>> +
>> + page = __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 underestimate
>> + * problem.
>> + */
>> + if (pool->frag_offset + *size > max_size) {
>> + *size = max_size - *offset;
>> + pool->frag_offset = 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 = size;
struct page *page;
int offset;

page = 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?

>