Re: [PATCH v2] net/af_xdp: fix umem map size for zero copy

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 5/22/2024 2:25 AM, Du, Frank wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Wednesday, May 22, 2024 1:58 AM
>> To: Du, Frank <frank.du@intel.com>; dev@dpdk.org; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Morten Brørup
>> <mb@smartsharesystems.com>
>> Cc: Loftus, Ciara <ciara.loftus@intel.com>; Burakov, Anatoly
>> <anatoly.burakov@intel.com>
>> Subject: Re: [PATCH v2] net/af_xdp: fix umem map size for zero copy
>>
>> On 5/11/2024 6:26 AM, Frank Du wrote:
>>> The current calculation assumes that the mbufs are contiguous.
>>> However, this assumption is incorrect when the memory spans across a huge
>> page.
>>> Correct to directly read the size from the mempool memory chunks.
>>>
>>> Signed-off-by: Frank Du <frank.du@intel.com>
>>>
>>> ---
>>> v2:
>>> * Add virtual contiguous detect for for multiple memhdrs.
>>> ---
>>>  drivers/net/af_xdp/rte_eth_af_xdp.c | 34
>>> ++++++++++++++++++++++++-----
>>>  1 file changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> index 268a130c49..7456108d6d 100644
>>> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
>>> @@ -1039,16 +1039,35 @@ eth_link_update(struct rte_eth_dev *dev
>>> __rte_unused,  }
>>>
>>>  #if defined(XDP_UMEM_UNALIGNED_CHUNK_FLAG)
>>> -static inline uintptr_t get_base_addr(struct rte_mempool *mp,
>>> uint64_t *align)
>>> +static inline uintptr_t get_memhdr_info(struct rte_mempool *mp,
>>> +uint64_t *align, size_t *len)
>>>  {
>>> -	struct rte_mempool_memhdr *memhdr;
>>> +	struct rte_mempool_memhdr *memhdr, *next;
>>>  	uintptr_t memhdr_addr, aligned_addr;
>>> +	size_t memhdr_len = 0;
>>>
>>> +	/* get the mempool base addr and align */
>>>  	memhdr = STAILQ_FIRST(&mp->mem_list);
>>>  	memhdr_addr = (uintptr_t)memhdr->addr;
>>>  	aligned_addr = memhdr_addr & ~(getpagesize() - 1);
>>>  	*align = memhdr_addr - aligned_addr;
>>>
>>
>> I am aware this is not part of this patch, but as note, can't we use
>> 'RTE_ALIGN_FLOOR' to calculate aligned address.
> 
> Sure, will use RTE_ALIGN_FLOOR in next version.
> 
>>
>>
>>> +	memhdr_len += memhdr->len;
>>> +
>>> +	/* check if virtual contiguous memory for multiple memhdrs */
>>> +	next = STAILQ_NEXT(memhdr, next);
>>> +	while (next != NULL) {
>>> +		if ((uintptr_t)next->addr != (uintptr_t)memhdr->addr + memhdr-
>>> len) {
>>> +			AF_XDP_LOG(ERR, "memory chunks not virtual
>> contiguous, "
>>> +					"next: %p, cur: %p(len: %" PRId64
>> " )\n",
>>> +					next->addr, memhdr->addr, memhdr-
>>> len);
>>> +			return 0;
>>> +		}
>>>
>>
>> Isn't there a mempool flag that can help us figure out mempool is not IOVA
>> contiguous? Isn't it sufficient on its own?
> 
> Indeed, what we need to ascertain is whether it's contiguous in CPU virtual space, not IOVA. I haven't come across a flag specifically for CPU virtual contiguity. The major limitation in XDP is XSK UMEM only supports registering a single contiguous virtual memory area.
> 

'RTE_MEMPOOL_F_NO_IOVA_CONTIG' is the flag I was looking for. This flag
being *cleared* implies IOVA contiguous but not sure if it is
guaranteed, need to check.

And I may be wrong here, but as far as I remember in IOVA as VA mode,
process virtual address and IOVA address are same, so IOVA contiguous is
same as contiguous CPU virtual address.

>>
>>
>>> +		/* virtual contiguous */
>>> +		memhdr = next;
>>> +		memhdr_len += memhdr->len;
>>> +		next = STAILQ_NEXT(memhdr, next);
>>> +	}
>>>
>>> +	*len = memhdr_len;
>>>  	return aligned_addr;
>>>  }
>>>
>>
>> This function goes too much details of the mempool object, and any change in
>> mempool details has potential to break this code.
>>
>> @Andrew, @Morten, do you think does it make sense to have
>> 'rte_mempool_info_get()' kind of function, that provides at least address and
>> length of the mempool, and used here?
>>
>> This helps to hide internal details and complexity of the mempool for users.
>>
>>
>>>
>>> @@ -1125,6 +1144,7 @@ xsk_umem_info *xdp_umem_configure(struct
>> pmd_internals *internals,
>>>  	void *base_addr = NULL;
>>>  	struct rte_mempool *mb_pool = rxq->mb_pool;
>>>  	uint64_t umem_size, align = 0;
>>> +	size_t len = 0;
>>>
>>>  	if (internals->shared_umem) {
>>>  		if (get_shared_umem(rxq, internals->if_name, &umem) < 0) @@
>>> -1156,10 +1176,12 @@ xsk_umem_info *xdp_umem_configure(struct
>> pmd_internals *internals,
>>>  		}
>>>
>>>  		umem->mb_pool = mb_pool;
>>> -		base_addr = (void *)get_base_addr(mb_pool, &align);
>>> -		umem_size = (uint64_t)mb_pool->populated_size *
>>> -				(uint64_t)usr_config.frame_size +
>>> -				align;
>>> +		base_addr = (void *)get_memhdr_info(mb_pool, &align, &len);
>>>
>>
>> Is this calculation correct if mempool is not already aligned to page size?
>>
>> Like in an example page size is '0x1000', and "memhdr_addr = 0x000a1080"
>> returned aligned address is '0x000a1000', "base_addr = 0x000a1000"
>>
>> Any access between '0x000a1000' & '0x000a1080' is invalid. Is this expected?
> 
> Yes, since the XSK UMEM memory area requires page alignment. However, no need to worry; the memory pointer in the XSK TX/RX descriptor is obtained from the mbuf data area. We don’t have any chance to access the invalid range [0x000a1000: 0x000a1080] here.
> 

Thanks for clarification.