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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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"?

> > > > > 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.