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: Thursday, May 23, 2024 3:41 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: Thursday, 23 May 2024 08.56
> >
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Wednesday, May 22, 2024 3:27 PM
> > >
> > > > 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.
> 
> What does "the memory spans across a huge page" mean?
> 
> Should it be "the memory spans across multiple memory chunks"?

This does not pertain to multiple memory chunks but rather to mbuf memory. The scenario involves a single memory chunk utilizing multiple 2M pages. To ensure that each mbuf resides exclusively within a single page, there are deliberate spacing gaps when allocating mbufs across the 2M page boundaries.

> 
> > > > > > 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.
> 
> If there is exactly one memory chunk, it is virtual contiguous. It has one address
> and one length, so it must be.
> 
> If there are more than one memory chunk, I consider it unlikely that they are
> contiguous.
> Have you ever observed the opposite, i.e. a mempool with multiple memory
> chunks being virtual contiguous?
> 
> Iterating through the list does not seem safer to me, quite the opposite.
> Which future change are you trying to prepare for?
> 
> Keeping it simple is more likely to not break with future changes.

No, I haven't encountered a mempool with multiple memory chunks actually, not know how to construct such mempool. The initial approach was to return an error if multiple chunks were detected, and the iteration method was introduced later. I can revert to the original, simpler way.