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

[Yunsheng Lin]

On 2023/6/13 22:36, Alexander Duyck wrote:
> On Fri, Jun 9, 2023 at 6:20 AM Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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?


>