Re: [RFC net-next 05/15] page_pool: Extend memory provider operations

[Björn Töpel]

Stanislav Fomichev <sdf.kernel@xxxxxxxxx> writes:

> On 10/02, Björn Töpel wrote:
>> The current memory providers give memory that the CPU cannot read,
>> and they can always refill. An AF_XDP provider is different. The CPU
>> can read its memory, it has only the buffers that userspace puts in
>> the FILL ring, and its buffers go back to userspace instead of being
>> freed. It needs four things from page_pool:
>> 
>> - A CPU address for each net_iov. A provider can set the address of
>>   the first net_iov in its net_iov_area and the size each net_iov
>>   covers. netmem_address() then computes the address without a call
>>   into the provider; drivers call it for every packet. A
>>   net_iov_area without an address stays unreadable.
>>   page_pool_is_unreadable() is false for a readable provider.
>> 
>> - A way to say that the refill is not done. The new refill_done
>>   callback tells the driver whether it may stop refilling. A
>>   provider that still has buffers returns false, and the driver
>>   keeps NAPI scheduled.
>> 
>> - A way to give up a buffer without freeing it. Add a batched call
>>   that ends page_pool ownership of buffers that leave through the
>>   provider, for example to userspace. It clears the page_pool link,
>>   so a later page pool can take the buffer. Add the matching batched
>>   call that sets the link, and a DMA sync helper for drivers that
>>   post a buffer again directly.
>> 
>> - No buffer splitting, because a split buffer has several owners.
>>   Add MP_CAP_FRAG. page_pool refuses fragment allocation from a
>>   provider without it.
>> 
>> devmem and io_uring set MP_CAP_FRAG and keep their fragment
>> behaviour. Their refill paths now use the batched call that sets the
>> link.
>> 
>> Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxx>
>> ---
>>  include/net/netmem.h                    | 31 ++++++++++-
>>  include/net/page_pool/helpers.h         | 62 +++++++++++++++++++++-
>>  include/net/page_pool/memory_provider.h | 28 +++++-----
>>  io_uring/zcrx.c                         |  4 +-
>>  net/core/devmem.c                       |  7 ++-
>>  net/core/page_pool.c                    | 68 ++++++++++++++++++++-----
>>  6 files changed, 165 insertions(+), 35 deletions(-)
>> 
>> diff --git a/include/net/netmem.h b/include/net/netmem.h
>> index cc97611632dc..e6dff0b01581 100644
>> --- a/include/net/netmem.h
>> +++ b/include/net/netmem.h
>> @@ -105,6 +105,12 @@ struct net_iov_area {
>>  
>
> [..]
>
>>  	/* Offset into the dma-buf where this chunk starts.  */
>>  	unsigned long base_virtual;
>
> Will send a patch to drop this one, don't think we use it anymore..

Ok!

>> +	/* CPU address of the first net_iov's memory, or NULL when the CPU
>> +	 * cannot read the area. Each net_iov covers 1 << @niov_shift bytes.
>> +	 */
>> +	void *vaddr;
>
> Who is setting this? Somewhere in another patch? Can we keep it here?

xp_pp_create() sets vaddr in patch 9. I'll move that closer.

>> +	u8 niov_shift;
>
> If we need niov_shift at area level, let's move the one from binding here?
> Then the existing callers can do binding->area.niov_shift.

Good! I'll consolidate the binding's shift (and rebase on your
base_virtual cleanup)

>>  };
>>  
>>  static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
>> @@ -117,6 +123,23 @@ static inline unsigned int net_iov_idx(const struct net_iov *niov)
>>  	return niov - net_iov_owner(niov)->niovs;
>>  }
>>  
>> +static inline bool net_iov_is_readable(const struct net_iov *niov)
>> +{
>> +	return net_iov_owner(niov)->vaddr;
>> +}
>> +
>> +static inline void *net_iov_address(const struct net_iov *niov)
>> +{
>> +	const struct net_iov_area *area = net_iov_owner(niov);
>> +	unsigned long off;
>> +
>> +	if (!area->vaddr)
>> +		return NULL;
>> +
>> +	off = (unsigned long)net_iov_idx(niov) << area->niov_shift;
>> +	return area->vaddr + off;
>
> Or, alternatively, store dma_base in area and do:
>
> return niov->dma_addr - area.dma_base + vadd ?

UMEM pages don't have to have contiguous DMA addresses, so I'd keep the
index-based CPU offset.


Björn