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

["Du, Frank" <frank.du@intel.com>]

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Wednesday, May 22, 2024 3:27 PM
> To: Du, Frank <frank.du@intel.com>; Ferruh Yigit <ferruh.yigit@amd.com>;
> dev@dpdk.org; Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Burakov,
> Anatoly <anatoly.burakov@intel.com>
> Cc: Loftus, Ciara <ciara.loftus@intel.com>
> Subject: RE: [PATCH v2] net/af_xdp: fix umem map size for zero copy
> 
> > From: Du, Frank [mailto:frank.du@intel.com]
> > Sent: Wednesday, 22 May 2024 03.25
> >
> > > From: Ferruh Yigit <ferruh.yigit@amd.com>
> > > Sent: Wednesday, May 22, 2024 1:58 AM
> > >
> > > 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;
> 
> This is not a new bug; but if the mempool is not populated, memhdr is NULL here.

Thanks, will add a check later.

> 
> > > >  	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.
> 
> I would assume that the EAL memory manager merges free memory into
> contiguous chunks whenever possible.
> @Anatoly, please confirm?
> 
> If my assumption is correct, it means that if mp->nb_mem_chunks != 1, then the
> mempool is not virtual contiguous. And if mp->nb_mem_chunks == 1, then it is;
> there is no need to iterate through the memhdr list.

If this's true now, however, this assumption may not hold true in the future code change, iterating through the list may is a safer way as it carefully checks the virtual address without relying on any condition.