Re: [PATCH RFC net-next v4 5/9] libie: add Rx buffer management (via Page Pool)

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Yunsheng Lin <linyunsheng@huawei.com>
Date: Thu, 6 Jul 2023 20:47:28 +0800

> On 2023/7/5 23:55, Alexander Lobakin wrote:
> 
>> +/**
>> + * libie_rx_page_pool_create - create a PP with the default libie settings
>> + * @napi: &napi_struct covering this PP (no usage outside its poll loops)
>> + * @size: size of the PP, usually simply Rx queue len
>> + *
>> + * Returns &page_pool on success, casted -errno on failure.
>> + */
>> +struct page_pool *libie_rx_page_pool_create(struct napi_struct *napi,
>> +					    u32 size)
>> +{
>> +	struct page_pool_params pp = {
>> +		.flags		= PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
>> +		.order		= LIBIE_RX_PAGE_ORDER,
>> +		.pool_size	= size,
>> +		.nid		= NUMA_NO_NODE,
>> +		.dev		= napi->dev->dev.parent,
>> +		.napi		= napi,
>> +		.dma_dir	= DMA_FROM_DEVICE,
>> +		.offset		= LIBIE_SKB_HEADROOM,
> 
> I think it worth mentioning that the '.offset' is not really accurate
> when the page is split, as we do not really know what is the offset of
> the frag of a page except for the first frag.

Yeah, this is read as "offset from the start of the page or frag to the
actual frame start, i.e. its Ethernet header" or "this is just
xdp->data - xdp->data_hard_start".

> 
>> +	};
>> +	size_t truesize;
>> +
>> +	pp.max_len = libie_rx_sync_len(napi->dev, pp.offset);
>> +
>> +	/* "Wanted" truesize, passed to page_pool_dev_alloc() */
>> +	truesize = roundup_pow_of_two(SKB_HEAD_ALIGN(pp.offset + pp.max_len));
>> +	pp.init_arg = (void *)truesize;
> 
> I am not sure if it is correct to use pp.init_arg here, as it is supposed to
> be used along with init_callback. And if we want to change the implemetation

I know. I abused it to save 1 function argument :p It's safe since I
don't use init_callback (not an argument).
I was thinking also of having a union in PP params or even a new field
like "wanted true size", so that your function could even take values
from there in certain cases (e.g. if I pass 0 as parameter).

> of init_callback, we may stuck with it as the driver is using it very
> differently here.
> 
> Is it possible to pass the 'wanted true size' by adding a parameter for
> libie_rx_alloc()?

Yes, or I could store it somewhere on the ring, but looks uglier =\ This
one does as well to some degree, but at least hidden in the library and
doesn't show up in the drivers :D

> 
>> +
>> +	return page_pool_create(&pp);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(libie_rx_page_pool_create, LIBIE);

Thanks,
Olek