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 7B22945AE1; Tue, 8 Oct 2024 11:03:27 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0D8F74021F; Tue, 8 Oct 2024 11:03:27 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 822DB4021E for ; Tue, 8 Oct 2024 11:03:25 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 47D7D203F3; Tue, 8 Oct 2024 11:03:25 +0200 (CEST) Content-class: urn:content-classes:message Subject: RE: [PATCH v6 17/17] eal: add function attributes for allocation functions MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Date: Tue, 8 Oct 2024 11:03:24 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F791@smartserver.smartshare.dk> In-Reply-To: <20241002154429.64357-18-stephen@networkplumber.org> X-MS-Has-Attach: X-MS-TNEF-Correlator: X-MimeOLE: Produced By Microsoft Exchange V6.5 Thread-Topic: [PATCH v6 17/17] eal: add function attributes for allocation functions Thread-Index: AdsU4lmrYOCE3BV1Srq3+unV9YkSBQEfAjzQ References: <20240927204742.546164-1-stephen@networkplumber.org> <20241002154429.64357-1-stephen@networkplumber.org> <20241002154429.64357-18-stephen@networkplumber.org> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Stephen Hemminger" , Cc: "Chengwen Feng" , "Anatoly Burakov" , "Tyler Retzlaff" 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: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Wednesday, 2 October 2024 17.43 >=20 > The allocation functions take a alignment argument that > can be useful to hint the compiler optimizer. This is supported > by GCC and Clang but only useful with GCC because Clang gives > warning if alignment is 0. >=20 > Newer versions of GCC have a malloc attribute that can be used to find > mismatches between allocation and free; the typical problem caught is = a > pointer allocated with rte_malloc() that is then incorrectly freed > using > free(). The name of the DPDK wrapper macros for these attributes > are chosen to be similar to what GLIBC is using in cdefs.h. > Note: The rte_free function prototype was moved ahead of the = allocation > functions since the dealloc attribute now refers to it. >=20 > Signed-off-by: Stephen Hemminger > Acked-by: Chengwen Feng > Acked-by: Anatoly Burakov > --- I see many of my comments to v3 have already been addressed. Great minds = think alike. :-) > +/** > + * With recent GCC versions also able to track that proper > + * deallocator function is used for this pointer. > + */ > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >=3D 110000) > +#define __rte_dealloc(dealloc, argno) \ > + __attribute__((malloc(dealloc, argno))) > +#else > +#define __rte_dealloc(dealloc, argno) > +#endif A matter of taste... The name "__rte_malloc" is closely associated with the function name = "malloc()"; for consistency suggest naming this "__rte_free" or = "__rte_malloc_free". If named __rte_malloc_free, it could include the __rte_malloc (as in = previous versions of the patch). However, that might be confusing, so probably not a good idea. I prefer keeping the attributes separate, as in this version. > +++ b/lib/eal/include/rte_malloc.h > @@ -31,6 +31,26 @@ struct rte_malloc_socket_stats { > size_t heap_allocsz_bytes; /**< Total allocated bytes on heap */ > }; >=20 > +/** > + * Function attribut for prototypes that expect to release memory = with > rte_free() Typo: attribut -> attribute > + */ > +#define __rte_dealloc_free __rte_dealloc(rte_free, 1) Minor detail: I'm worried someone might misunderstand the purpose of this shortcut, = and use it with an allocator function where a different deallocator is = associated. Moving it from rte_common.h to rte_malloc.h is a huge improvement; but = please consider if the benefit outweighs the risk. Again, I'll leave it up to you.