DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] net/af_xdp: fix umem map size for zero copy
@ 2024-04-26  0:51 Frank Du
  2024-04-26 10:43 ` Loftus, Ciara
  2024-05-11  5:26 ` [PATCH v2] " Frank Du
  0 siblings, 2 replies; 7+ messages in thread
From: Frank Du @ 2024-04-26  0:51 UTC (permalink / raw)
  To: dev

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>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 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..cb95d17d13 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -1039,7 +1039,7 @@ 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;
 	uintptr_t memhdr_addr, aligned_addr;
@@ -1048,6 +1048,7 @@ static inline uintptr_t get_base_addr(struct rte_mempool *mp, uint64_t *align)
 	memhdr_addr = (uintptr_t)memhdr->addr;
 	aligned_addr = memhdr_addr & ~(getpagesize() - 1);
 	*align = memhdr_addr - aligned_addr;
+	*len = memhdr->len;
 
 	return aligned_addr;
 }
@@ -1125,6 +1126,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 +1158,8 @@ 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);
+		umem_size = (uint64_t)len + align;
 
 		ret = xsk_umem__create(&umem->umem, base_addr, umem_size,
 				&rxq->fq, &rxq->cq, &usr_config);
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_xdp: fix umem map size for zero copy
  2024-04-26  0:51 [PATCH] net/af_xdp: fix umem map size for zero copy Frank Du
@ 2024-04-26 10:43 ` Loftus, Ciara
  2024-04-28  0:46   ` Du, Frank
  2024-05-11  5:26 ` [PATCH v2] " Frank Du
  1 sibling, 1 reply; 7+ messages in thread
From: Loftus, Ciara @ 2024-04-26 10:43 UTC (permalink / raw)
  To: Du, Frank; +Cc: dev

> Subject: [PATCH] net/af_xdp: fix umem map size for zero copy
> 
> 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>
> ---
>  drivers/net/af_xdp/rte_eth_af_xdp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 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..cb95d17d13 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -1039,7 +1039,7 @@ 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;
>  	uintptr_t memhdr_addr, aligned_addr;
> @@ -1048,6 +1048,7 @@ static inline uintptr_t get_base_addr(struct
> rte_mempool *mp, uint64_t *align)
>  	memhdr_addr = (uintptr_t)memhdr->addr;
>  	aligned_addr = memhdr_addr & ~(getpagesize() - 1);
>  	*align = memhdr_addr - aligned_addr;
> +	*len = memhdr->len;
> 
>  	return aligned_addr;
>  }
> @@ -1125,6 +1126,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 +1158,8 @@ 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);
> +		umem_size = (uint64_t)len + align;

len is set to the length of the first memhdr of the mempool. There may be many other memhdrs in the mempool. So I don't think this is the correct value to use for calculating the entire umem size.

> 
>  		ret = xsk_umem__create(&umem->umem, base_addr,
> umem_size,
>  				&rxq->fq, &rxq->cq, &usr_config);
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_xdp: fix umem map size for zero copy
  2024-04-26 10:43 ` Loftus, Ciara
@ 2024-04-28  0:46   ` Du, Frank
  2024-04-30  9:22     ` Loftus, Ciara
  0 siblings, 1 reply; 7+ messages in thread
From: Du, Frank @ 2024-04-28  0:46 UTC (permalink / raw)
  To: Loftus, Ciara; +Cc: dev

> -----Original Message-----
> From: Loftus, Ciara <ciara.loftus@intel.com>
> Sent: Friday, April 26, 2024 6:44 PM
> To: Du, Frank <frank.du@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH] net/af_xdp: fix umem map size for zero copy
> 
> > Subject: [PATCH] net/af_xdp: fix umem map size for zero copy
> >
> > 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>
> > ---
> >  drivers/net/af_xdp/rte_eth_af_xdp.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 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..cb95d17d13 100644
> > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > @@ -1039,7 +1039,7 @@ 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;
> >  	uintptr_t memhdr_addr, aligned_addr; @@ -1048,6 +1048,7 @@ static
> > inline uintptr_t get_base_addr(struct rte_mempool *mp, uint64_t
> > *align)
> >  	memhdr_addr = (uintptr_t)memhdr->addr;
> >  	aligned_addr = memhdr_addr & ~(getpagesize() - 1);
> >  	*align = memhdr_addr - aligned_addr;
> > +	*len = memhdr->len;
> >
> >  	return aligned_addr;
> >  }
> > @@ -1125,6 +1126,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 +1158,8 @@ 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);
> > +		umem_size = (uint64_t)len + align;
> 
> len is set to the length of the first memhdr of the mempool. There may be many
> other memhdrs in the mempool. So I don't think this is the correct value to use for
> calculating the entire umem size.

Current each xdp rx ring is bonded to one single umem region, it can't reuse the memory
if there are multiple memhdrs in the mempool. How about adding a check on the number
of the memory chunks to only allow one single memhdr mempool can be used here?

> 
> >
> >  		ret = xsk_umem__create(&umem->umem, base_addr,
> umem_size,
> >  				&rxq->fq, &rxq->cq, &usr_config);
> > --
> > 2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH] net/af_xdp: fix umem map size for zero copy
  2024-04-28  0:46   ` Du, Frank
@ 2024-04-30  9:22     ` Loftus, Ciara
  0 siblings, 0 replies; 7+ messages in thread
From: Loftus, Ciara @ 2024-04-30  9:22 UTC (permalink / raw)
  To: Du, Frank; +Cc: dev

> >
> > > Subject: [PATCH] net/af_xdp: fix umem map size for zero copy
> > >
> > > 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>
> > > ---
> > >  drivers/net/af_xdp/rte_eth_af_xdp.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 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..cb95d17d13 100644
> > > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> > > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > > @@ -1039,7 +1039,7 @@ 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;
> > >  	uintptr_t memhdr_addr, aligned_addr; @@ -1048,6 +1048,7 @@
> static
> > > inline uintptr_t get_base_addr(struct rte_mempool *mp, uint64_t
> > > *align)
> > >  	memhdr_addr = (uintptr_t)memhdr->addr;
> > >  	aligned_addr = memhdr_addr & ~(getpagesize() - 1);
> > >  	*align = memhdr_addr - aligned_addr;
> > > +	*len = memhdr->len;
> > >
> > >  	return aligned_addr;
> > >  }
> > > @@ -1125,6 +1126,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 +1158,8 @@ 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);
> > > +		umem_size = (uint64_t)len + align;
> >
> > len is set to the length of the first memhdr of the mempool. There may be
> many
> > other memhdrs in the mempool. So I don't think this is the correct value to
> use for
> > calculating the entire umem size.
> 
> Current each xdp rx ring is bonded to one single umem region, it can't reuse
> the memory
> if there are multiple memhdrs in the mempool. How about adding a check on
> the number
> of the memory chunks to only allow one single memhdr mempool can be used
> here?

The UMEM needs to be a region of virtual contiguous memory. I think this can still be the case, even if the mempool has multiple memhdrs.
If we detect >1 memhdrs perhaps we need to verify that the RTE_MEMPOOL_F_NO_IOVA_CONTIG flag is not set which I think would mean that the mempool may not be virtually contiguous.

> 
> >
> > >
> > >  		ret = xsk_umem__create(&umem->umem, base_addr,
> > umem_size,
> > >  				&rxq->fq, &rxq->cq, &usr_config);
> > > --
> > > 2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] net/af_xdp: fix umem map size for zero copy
  2024-04-26  0:51 [PATCH] net/af_xdp: fix umem map size for zero copy Frank Du
  2024-04-26 10:43 ` Loftus, Ciara
@ 2024-05-11  5:26 ` Frank Du
  2024-05-17 13:19   ` Loftus, Ciara
  1 sibling, 1 reply; 7+ messages in thread
From: Frank Du @ 2024-05-11  5:26 UTC (permalink / raw)
  To: dev; +Cc: ciara.loftus

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;
+	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;
+		}
+		/* virtual contiguous */
+		memhdr = next;
+		memhdr_len += memhdr->len;
+		next = STAILQ_NEXT(memhdr, next);
+	}
 
+	*len = memhdr_len;
 	return aligned_addr;
 }
 
@@ -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);
+		if (!base_addr) {
+			AF_XDP_LOG(ERR, "Failed to parse memhdr info from pool\n");
+			goto err;
+		}
+		umem_size = (uint64_t)len + align;
 
 		ret = xsk_umem__create(&umem->umem, base_addr, umem_size,
 				&rxq->fq, &rxq->cq, &usr_config);
-- 
2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v2] net/af_xdp: fix umem map size for zero copy
  2024-05-11  5:26 ` [PATCH v2] " Frank Du
@ 2024-05-17 13:19   ` Loftus, Ciara
  2024-05-20  1:28     ` Du, Frank
  0 siblings, 1 reply; 7+ messages in thread
From: Loftus, Ciara @ 2024-05-17 13:19 UTC (permalink / raw)
  To: Du, Frank; +Cc: dev

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

Hi Frank,

Thanks for the patch.

Before your patch the umem_size was calculated using mb_pool->populated_size * rte_mempool_calc_obj_size(mb_pool->elt_size, ..)
With your patch we sum up the lens of all memhdrs in the mempool.

When debugging I see the new calculation can yield a larger value, but the new logic looks good and more thorough to me so I'm happy to opt with the new approach.

Acked-by: Ciara Loftus <ciara.loftus@intel.com>

Thanks,
Ciara

> 
> ---
> 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;
> +	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;
> +		}
> +		/* virtual contiguous */
> +		memhdr = next;
> +		memhdr_len += memhdr->len;
> +		next = STAILQ_NEXT(memhdr, next);
> +	}
> 
> +	*len = memhdr_len;
>  	return aligned_addr;
>  }
> 
> @@ -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);
> +		if (!base_addr) {
> +			AF_XDP_LOG(ERR, "Failed to parse memhdr info from
> pool\n");
> +			goto err;
> +		}
> +		umem_size = (uint64_t)len + align;
> 
>  		ret = xsk_umem__create(&umem->umem, base_addr,
> umem_size,
>  				&rxq->fq, &rxq->cq, &usr_config);
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH v2] net/af_xdp: fix umem map size for zero copy
  2024-05-17 13:19   ` Loftus, Ciara
@ 2024-05-20  1:28     ` Du, Frank
  0 siblings, 0 replies; 7+ messages in thread
From: Du, Frank @ 2024-05-20  1:28 UTC (permalink / raw)
  To: Loftus, Ciara; +Cc: dev

> -----Original Message-----
> From: Loftus, Ciara <ciara.loftus@intel.com>
> Sent: Friday, May 17, 2024 9:20 PM
> To: Du, Frank <frank.du@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] net/af_xdp: fix umem map size for zero copy
> 
> >
> > 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>
> 
> Hi Frank,
> 
> Thanks for the patch.
> 
> Before your patch the umem_size was calculated using mb_pool->populated_size
> * rte_mempool_calc_obj_size(mb_pool->elt_size, ..) With your patch we sum up
> the lens of all memhdrs in the mempool.
> 
> When debugging I see the new calculation can yield a larger value, but the new
> logic looks good and more thorough to me so I'm happy to opt with the new
> approach.

Hi Ciara,

Thanks for the review. The reason for a larger value is that, the pool contain some necessary space hole to ensure one single mbuf does not span two virtual huge page, so that the PA of each mbuf is secure for the hardware operation.

> 
> Acked-by: Ciara Loftus <ciara.loftus@intel.com>
> 
> Thanks,
> Ciara
> 
> >
> > ---
> > 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;
> > +	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;
> > +		}
> > +		/* virtual contiguous */
> > +		memhdr = next;
> > +		memhdr_len += memhdr->len;
> > +		next = STAILQ_NEXT(memhdr, next);
> > +	}
> >
> > +	*len = memhdr_len;
> >  	return aligned_addr;
> >  }
> >
> > @@ -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);
> > +		if (!base_addr) {
> > +			AF_XDP_LOG(ERR, "Failed to parse memhdr info from
> > pool\n");
> > +			goto err;
> > +		}
> > +		umem_size = (uint64_t)len + align;
> >
> >  		ret = xsk_umem__create(&umem->umem, base_addr,
> umem_size,
> >  				&rxq->fq, &rxq->cq, &usr_config);
> > --
> > 2.34.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-05-20  1:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-26  0:51 [PATCH] net/af_xdp: fix umem map size for zero copy Frank Du
2024-04-26 10:43 ` Loftus, Ciara
2024-04-28  0:46   ` Du, Frank
2024-04-30  9:22     ` Loftus, Ciara
2024-05-11  5:26 ` [PATCH v2] " Frank Du
2024-05-17 13:19   ` Loftus, Ciara
2024-05-20  1:28     ` Du, Frank

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).