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 86227A00C3; Tue, 29 Nov 2022 17:59:16 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 69A5540691; Tue, 29 Nov 2022 17:59:16 +0100 (CET) Received: from mail-yb1-f171.google.com (mail-yb1-f171.google.com [209.85.219.171]) by mails.dpdk.org (Postfix) with ESMTP id 733974067E for ; Tue, 29 Nov 2022 10:57:17 +0100 (CET) Received: by mail-yb1-f171.google.com with SMTP id j196so16780322ybj.2 for ; Tue, 29 Nov 2022 01:57:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=tLsiVKW/HV801jViJ+au18+LG2adVtog33T+DHW5elk=; b=Kd31Pc/Lq4B7cizqJW0oMOpAfpT/uAfoVDeQltgyEQueaUh3JcpkXtTR9Co1+5KaK/ 51YpIcMmEWWlMwxgvbO7RyspDJwQT5mUu1nriGZtScMflgmfAERo4RAOxueLQgTAXiaJ qiQ1dhRXBG7zGhqEHjW529laZdMgOHfWoyJctc1hk/B1ruWb4IGegF7BNRHm/LF8032u 3LIyswWVbahZ8IDtLek/q7YWA49RsmFHxDnDPu2kj4L6YpBMvAk8dgU6ExnUYwqcj+Cu fqiDgmmTqB4t3qjk4m9Ghds0/n/sZKmY1ncaCo0YZ07saM+eUTofhr4rNX0NS1ONJT7S o7aQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=tLsiVKW/HV801jViJ+au18+LG2adVtog33T+DHW5elk=; b=EDxKpWQsXuklWGxW/kcs3ndW0sH8T3AVMpiE4u0zPJFxnYOAu+GfgrItybaZOaZbaJ KKIHqYuW06wftjFaCk5L1yFjXoFy1a09W0LyIvUXSl+YPh0VQiMowSKPQFrOEmXDC8wu kD0qZA77PQr9w+31lWVx8A5JHZRO1t8zS0DZ/8aa0hkw77W60QO0UiEIJRsFc7lHxvcH zg3uxVfNwRpwNhjrHBHp+/dWlXZsMO1gULP1+IxWw0O+C2zdV36y09aRLFORfhvh/6Bh zQFZfItme+9MwUgYHRha0O4zs7RgBU4oE5ee7TcsFhKiT3hqJv+kBHYX6d9nxyaOGl8A 9x9g== X-Gm-Message-State: ANoB5pk5Ib8KBYyCKVZxQYhSwdSdQrF2H9HV5ZvZjjRz7O1bnZWEQH5M JyEARV3IoceWMhnreFC0DlxdXEaEmQR74AKuDMnWzcdwauc48Q== X-Google-Smtp-Source: AA0mqf6PFqtprzF2wvsvaOSZxp9htcERQ9pZS8iPAIOSJkQyWlR/+C5uyIVMG94p2bSQpwjKM+HYa2+ntv75cvcWRbs= X-Received: by 2002:a05:6902:1824:b0:6ca:1f82:abbc with SMTP id cf36-20020a056902182400b006ca1f82abbcmr33505058ybb.429.1669715836633; Tue, 29 Nov 2022 01:57:16 -0800 (PST) MIME-Version: 1.0 References: <20221115123502.12560-1-changfengnan@bytedance.com> In-Reply-To: From: Fengnan Chang Date: Tue, 29 Nov 2022 17:57:05 +0800 Message-ID: Subject: Re: [External] Re: [PATCH v2] mempool: fix rte_mempool_avail_count may segment fault when used in multiprocess To: Olivier Matz Cc: david.marchand@redhat.com, mb@smartsharesystems.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailman-Approved-At: Tue, 29 Nov 2022 17:59:15 +0100 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 Olivier Matz =E4=BA=8E2022=E5=B9=B411=E6=9C=8822= =E6=97=A5=E5=91=A8=E4=BA=8C 23:25=E5=86=99=E9=81=93=EF=BC=9A > > Hi, > > On Tue, Nov 15, 2022 at 08:35:02PM +0800, Fengnan Chang wrote: > > rte_mempool_create put tailq entry into rte_mempool_tailq list before > > populate, and pool_data set when populate. So in multi process, if > > process A create mempool, and process B can get mempool through > > rte_mempool_lookup before pool_data set, if B call rte_mempool_avail_co= unt, > > it will cause segment fault. > > > > Fix this by put tailq entry into rte_mempool_tailq after populate. > > > > Signed-off-by: Fengnan Chang > > --- > > lib/mempool/rte_mempool.c | 43 ++++++++++++++++++++++----------------- > > 1 file changed, 24 insertions(+), 19 deletions(-) > > > > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c > > index 4c78071a34..b3a6572fc8 100644 > > --- a/lib/mempool/rte_mempool.c > > +++ b/lib/mempool/rte_mempool.c > > @@ -155,6 +155,27 @@ get_min_page_size(int socket_id) > > return wa.min =3D=3D SIZE_MAX ? (size_t) rte_mem_page_size() : wa= .min; > > } > > > > +static int > > +add_mempool_to_list(struct rte_mempool *mp) > > +{ > > + struct rte_mempool_list *mempool_list; > > + struct rte_tailq_entry *te =3D NULL; > > + > > + /* try to allocate tailq entry */ > > + te =3D rte_zmalloc("MEMPOOL_TAILQ_ENTRY", sizeof(*te), 0); > > + if (te =3D=3D NULL) { > > + RTE_LOG(ERR, MEMPOOL, "Cannot allocate tailq entry!\n"); > > + return -ENOMEM; > > + } > > + > > + te->data =3D mp; > > + mempool_list =3D RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempo= ol_list); > > + rte_mcfg_tailq_write_lock(); > > + TAILQ_INSERT_TAIL(mempool_list, te, next); > > + rte_mcfg_tailq_write_unlock(); > > + > > + return 0; > > +} > > > > static void > > mempool_add_elem(struct rte_mempool *mp, __rte_unused void *opaque, > > @@ -304,6 +325,9 @@ mempool_ops_alloc_once(struct rte_mempool *mp) > > if (ret !=3D 0) > > return ret; > > mp->flags |=3D RTE_MEMPOOL_F_POOL_CREATED; > > + ret =3D add_mempool_to_list(mp); > > + if (ret !=3D 0) > > + return ret; > > One issue here is that if the rte_zmalloc("MEMPOOL_TAILQ_ENTRY") fails, > the function will fail, but rte_mempool_ops_alloc() may already be > successful. > > I agree it's theorical, because an allocation failure would cause more > issues at the end. But, to be rigorous, I think we should do something > like this instead (not tested, just for the idea): > > static int > mempool_ops_alloc_once(struct rte_mempool *mp) > { > struct rte_mempool_list *mempool_list; > struct rte_tailq_entry *te =3D NULL; > int ret; > > /* only create the driver ops and add in tailq in if not = already done */ > if ((mp->flags & RTE_MEMPOOL_F_POOL_CREATED)) > return 0; > > te =3D rte_zmalloc("MEMPOOL_TAILQ_ENTRY", sizeof(*te), 0)= ; > if (te =3D=3D NULL) { > RTE_LOG(ERR, MEMPOOL, "Cannot allocate tailq entr= y!\n"); > ret =3D -rte_errno; > goto fail; > } > te->data =3D mp; > mempool_list =3D RTE_TAILQ_CAST(rte_mempool_tailq.head, r= te_mempool_list); > > ret =3D rte_mempool_ops_alloc(mp); > if (ret !=3D 0) > goto fail; > > mp->flags |=3D RTE_MEMPOOL_F_POOL_CREATED; > rte_mcfg_tailq_write_lock(); > TAILQ_INSERT_TAIL(mempool_list, te, next); > rte_mcfg_tailq_write_unlock(); > > return 0; > > fail: > rte_free(te); > return ret; > } > > > Thinking a bit more about the problem itself: the segfault that you > describe could also happen in a primary, without multi-process: > - create an empty mempool > - call rte_mempool_avail_count() before it is populated > > This simply means that an empty mempool is not ready for use, until > rte_mempool_set_ops_byname() or rte_mempool_populate*() is called. This > is something that we should document above the declaration of > rte_mempool_create_empty(). We could also say there that the mempool > will become visible to the secondary processes as soon as the driver ops > are set. > > However I still believe that a better synchronization point is required > in the application. After all, the presence in the TAILQ does not give > any hint on the status of the object. Can we imagine a case where a > mempool is created empty in a primary, and populated in a secondary? If > such use-case exist, we may not want to take this patch. Maybe there is a case like you said, do you think adding check mempool flag= s in rte_mempool_avail_count is acceptable ? If RTE_MEMPOOL_F_POOL_CREATED not set, just return 0. > > > } > > return 0; > > } > > @@ -798,9 +822,7 @@ rte_mempool_create_empty(const char *name, unsigned= n, unsigned elt_size, > > int socket_id, unsigned flags) > > { > > char mz_name[RTE_MEMZONE_NAMESIZE]; > > - struct rte_mempool_list *mempool_list; > > struct rte_mempool *mp =3D NULL; > > - struct rte_tailq_entry *te =3D NULL; > > const struct rte_memzone *mz =3D NULL; > > size_t mempool_size; > > unsigned int mz_flags =3D RTE_MEMZONE_1GB|RTE_MEMZONE_SIZE_HINT_O= NLY; > > @@ -820,8 +842,6 @@ rte_mempool_create_empty(const char *name, unsigned= n, unsigned elt_size, > > RTE_CACHE_LINE_MASK) !=3D 0); > > #endif > > > > - mempool_list =3D RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempo= ol_list); > > - > > /* asked for zero items */ > > if (n =3D=3D 0) { > > rte_errno =3D EINVAL; > > @@ -866,14 +886,6 @@ rte_mempool_create_empty(const char *name, unsigne= d n, unsigned elt_size, > > private_data_size =3D (private_data_size + > > RTE_MEMPOOL_ALIGN_MASK) & (~RTE_MEMPOOL_ALIG= N_MASK); > > > > - > > - /* try to allocate tailq entry */ > > - te =3D rte_zmalloc("MEMPOOL_TAILQ_ENTRY", sizeof(*te), 0); > > - if (te =3D=3D NULL) { > > - RTE_LOG(ERR, MEMPOOL, "Cannot allocate tailq entry!\n"); > > - goto exit_unlock; > > - } > > - > > mempool_size =3D RTE_MEMPOOL_HEADER_SIZE(mp, cache_size); > > mempool_size +=3D private_data_size; > > mempool_size =3D RTE_ALIGN_CEIL(mempool_size, RTE_MEMPOOL_ALIGN); > > @@ -923,20 +935,13 @@ rte_mempool_create_empty(const char *name, unsign= ed n, unsigned elt_size, > > cache_size); > > } > > > > - te->data =3D mp; > > - > > - rte_mcfg_tailq_write_lock(); > > - TAILQ_INSERT_TAIL(mempool_list, te, next); > > - rte_mcfg_tailq_write_unlock(); > > rte_mcfg_mempool_write_unlock(); > > - > > rte_mempool_trace_create_empty(name, n, elt_size, cache_size, > > private_data_size, flags, mp); > > return mp; > > > > exit_unlock: > > rte_mcfg_mempool_write_unlock(); > > - rte_free(te); > > rte_mempool_free(mp); > > return NULL; > > } > > -- > > 2.37.0 (Apple Git-136) > >