From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 14BAC440A4; Thu, 23 May 2024 11:22:44 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DEC7D402DD; Thu, 23 May 2024 11:22:43 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id E26C7402BF for ; Thu, 23 May 2024 11:22:42 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id BF86020568; Thu, 23 May 2024 11:22:42 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v4] net/af_xdp: fix umem map size for zero copy X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Thu, 23 May 2024 11:22:37 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F492@smartserver.smartshare.dk> In-Reply-To: <20240523080751.2347970-1-frank.du@intel.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v4] net/af_xdp: fix umem map size for zero copy Thread-Index: Adqs6s6ca8WHiTGpRNSZScY++riLDQAAP4BA References: <20240426005128.148730-1-frank.du@intel.com> <20240523080751.2347970-1-frank.du@intel.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Frank Du" , Cc: , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Frank Du [mailto:frank.du@intel.com] > Sent: Thursday, 23 May 2024 10.08 >=20 > The current calculation assumes that the mbufs are contiguous. = However, > this assumption is incorrect when the mbuf memory spans across huge = page. > To ensure that each mbuf resides exclusively within a single page, = there > are deliberate spacing gaps when allocating mbufs across the = boundaries. A agree that this patch is an improvement of what existed previously. But I still don't understand the patch description. To me, it looks like = the patch adds a missing check for contiguous memory, and the patch = itself has nothing to do with huge pages. Anyway, if the maintainer = agrees with the description, I don't mind not grasping it. ;-) However, while trying to understand what is happening, I think I found = one more (already existing) bug. I will show through an example inline below. >=20 > Correct to directly read the size from the mempool memory chunk. >=20 > Fixes: d8a210774e1d ("net/af_xdp: support unaligned umem chunks") > Cc: stable@dpdk.org >=20 > Signed-off-by: Frank Du >=20 > --- > v2: > * Add virtual contiguous detect for for multiple memhdrs > v3: > * Use RTE_ALIGN_FLOOR to get the aligned addr > * Add check on the first memhdr of memory chunks > v4: > * Replace the iterating with simple nb_mem_chunks check > --- > drivers/net/af_xdp/rte_eth_af_xdp.c | 33 = +++++++++++++++++++++++------ > 1 file changed, 26 insertions(+), 7 deletions(-) >=20 > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c > b/drivers/net/af_xdp/rte_eth_af_xdp.c > index 6ba455bb9b..d0431ec089 100644 > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c > @@ -1040,16 +1040,32 @@ eth_link_update(struct rte_eth_dev *dev = __rte_unused, > } >=20 > #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(const struct rte_mempool *mp, uint64_t *align, size_t = *len) > { > struct rte_mempool_memhdr *memhdr; > uintptr_t memhdr_addr, aligned_addr; >=20 > + if (mp->nb_mem_chunks !=3D 1) { > + /* > + * The mempool with multiple chunks is not virtual contiguous but > + * xsk umem only support single virtual region mapping. > + */ > + AF_XDP_LOG(ERR, "The mempool contain multiple %u memory > chunks\n", > + mp->nb_mem_chunks); > + return 0; > + } > + > + /* Get the mempool base addr and align from the header now */ > memhdr =3D STAILQ_FIRST(&mp->mem_list); > + if (!memhdr) { > + AF_XDP_LOG(ERR, "The mempool is not populated\n"); > + return 0; > + } > memhdr_addr =3D (uintptr_t)memhdr->addr; > - aligned_addr =3D memhdr_addr & ~(getpagesize() - 1); > + aligned_addr =3D RTE_ALIGN_FLOOR(memhdr_addr, getpagesize()); > *align =3D memhdr_addr - aligned_addr; > - > + *len =3D memhdr->len; > return aligned_addr; On x86_64, the page size is 4 KB =3D 0x1000. Let's look at an example where memhdr->addr is not aligned to the page = size: In the example, memhdr->addr is 0x700100, and memhdr->len is 0x20000. Then aligned_addr becomes 0x700000, *align becomes 0x100, and *len becomes 0x20000. > } >=20 > @@ -1126,6 +1142,7 @@ xsk_umem_info *xdp_umem_configure(struct = pmd_internals > *internals, > void *base_addr =3D NULL; > struct rte_mempool *mb_pool =3D rxq->mb_pool; > uint64_t umem_size, align =3D 0; > + size_t len =3D 0; >=20 > if (internals->shared_umem) { > if (get_shared_umem(rxq, internals->if_name, &umem) < 0) > @@ -1157,10 +1174,12 @@ xsk_umem_info *xdp_umem_configure(struct = pmd_internals > *internals, > } >=20 > umem->mb_pool =3D mb_pool; > - base_addr =3D (void *)get_base_addr(mb_pool, &align); > - umem_size =3D (uint64_t)mb_pool->populated_size * > - (uint64_t)usr_config.frame_size + > - align; > + base_addr =3D (void *)get_memhdr_info(mb_pool, &align, &len); > + if (!base_addr) { > + AF_XDP_LOG(ERR, "The memory pool can't be mapped as > umem\n"); > + goto err; > + } > + umem_size =3D (uint64_t)len + align; Here, umem_size becomes 0x20100. >=20 > ret =3D xsk_umem__create(&umem->umem, base_addr, umem_size, > &rxq->fq, &rxq->cq, &usr_config); Here, xsk_umem__create() is called with the base_address (0x700000) = preceding the address of the memory chunk (0x700100). It looks like a bug, causing a buffer underrun. I.e. will it access = memory starting at base_address? If I'm correct, the code should probably do this for alignment instead: aligned_addr =3D RTE_ALIGN_CEIL(memhdr_addr, getpagesize()); *align =3D aligned_addr - memhdr_addr; umem_size =3D (uint64_t)len - align; Disclaimer: I don't know much about the AF_XDP implementation, so maybe = I just don't understand what is going on. > -- > 2.34.1