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 D634541E65; Fri, 10 Mar 2023 15:06:23 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 85B4840A81; Fri, 10 Mar 2023 15:06:23 +0100 (CET) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id D938140685 for ; Fri, 10 Mar 2023 15:06:21 +0100 (CET) Received: from frapeml100007.china.huawei.com (unknown [172.18.147.200]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4PY79y58pkz6J69p; Fri, 10 Mar 2023 22:05:42 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml100007.china.huawei.com (7.182.85.133) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Fri, 10 Mar 2023 15:06:20 +0100 Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2507.021; Fri, 10 Mar 2023 15:06:20 +0100 From: Konstantin Ananyev To: Honnappa Nagarahalli , "mb@smartsharesystems.com" , "olivier.matz@6wind.com" , "konstantin.v.ananyev@yandex.ru" CC: "dev@dpdk.org" , Ruifeng Wang , Kamalakshitha Aligeri , "Wathsala Wathawana Vithanage" , nd , nd Subject: RE: [PATCH 4/4] mempool: use lcore API to check if lcore ID is valid Thread-Topic: [PATCH 4/4] mempool: use lcore API to check if lcore ID is valid Thread-Index: AQHZUkPdMQ5VovBe6ku0RkzWFw9njK7yL+WggAEx5KCAAKND8A== Date: Fri, 10 Mar 2023 14:06:20 +0000 Message-ID: References: <20230309045738.1261000-1-honnappa.nagarahalli@arm.com> <20230309045738.1261000-5-honnappa.nagarahalli@arm.com> <871c50be83964a08b7c2122d1694c80d@huawei.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.206.138.42] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-CFilter-Loop: Reflected 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 >=20 > > > > > > > > > > > > > Use lcore API to check if the lcore ID is valid. The runtime check > > > does not add much value. > > > > From my perspective it adds a perfect value: > > Only threads with valid lcore id have their own default mempool cache. > The threads would call 'rte_lcore_id()' to return their lcore_id. This en= sures the lcore_id is valid already. > Why do we need to check it > again in rte_mempool_default_cache? Why would a thread use an incorrect l= core_id? rte_lcore_id() will just return you value of per-thread variable: RTE_PER_L= CORE(_lcore_id). Without any checking. For non-eal tthreads this value would be UINT32_MAX. =20 > > > > > Hence use assert to validate > > > the lcore ID. > > > > Wonder why? > > What's wrong for the thread to try to get default mempool cache? > What are the cases where a thread does not know that it is not an EAL thr= ead and call rte_mempool_default_cache with a random > lcore_id? > Since, this API is called in the data plane, it makes sense to remove any= additional validations. Why is that? I believe this API have to be generic and safe to call from any thread. Let's look for example at: */ static __rte_always_inline void rte_mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table, unsigned int n) { struct rte_mempool_cache *cache; cache =3D rte_mempool_default_cache(mp, rte_lcore_id()); rte_mempool_trace_put_bulk(mp, obj_table, n, cache); rte_mempool_generic_put(mp, obj_table, n, cache); }=20 Right now it is perfectly valid to invoke it from any thread. With the change you propose invoking mempool_put() from non-EAL thread will cause either a crash or silent memory corruption. =20 > > That would change existing behavior and in general seems wrong to me. > Agree on the change in existing behavior. We can discuss this once we agr= ee/disagree on the above. >=20 > > So I am strongly opposed. > > > > > Signed-off-by: Honnappa Nagarahalli > > > Reviewed-by: Wathsala Vithanage > > > Reviewed-by: Ruifeng Wang > > > --- > > > lib/mempool/rte_mempool.h | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h > > > index 009bd10215..00c5aa961b 100644 > > > --- a/lib/mempool/rte_mempool.h > > > +++ b/lib/mempool/rte_mempool.h > > > @@ -1314,10 +1314,9 @@ rte_mempool_cache_free(struct > > rte_mempool_cache > > > *cache); static __rte_always_inline struct rte_mempool_cache * > > > rte_mempool_default_cache(struct rte_mempool *mp, unsigned lcore_id) > > > { > > > - if (mp->cache_size =3D=3D 0) > > > - return NULL; > > > + RTE_ASSERT(rte_lcore_id_is_valid(lcore_id)); > > > > > > - if (lcore_id >=3D RTE_MAX_LCORE) > > > + if (mp->cache_size =3D=3D 0) > > > return NULL; > > > > > > rte_mempool_trace_default_cache(mp, lcore_id, > > > -- > > > 2.25.1 > > >