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 8880D45AE1; Tue, 8 Oct 2024 10:29:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7894D40655; Tue, 8 Oct 2024 10:29:26 +0200 (CEST) Received: from dkmailrelay1.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 667764021F for ; Tue, 8 Oct 2024 10:29:24 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesys.local [192.168.4.10]) by dkmailrelay1.smartsharesystems.com (Postfix) with ESMTP id 4B596203F3; Tue, 8 Oct 2024 10:29:24 +0200 (CEST) Content-class: urn:content-classes:message Subject: RE: [PATCH v3 17/18] 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 10:29:23 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35E9F790@smartserver.smartshare.dk> In-Reply-To: <20240929154107.62539-18-stephen@networkplumber.org> X-MS-Has-Attach: X-MS-TNEF-Correlator: X-MimeOLE: Produced By Microsoft Exchange V6.5 Thread-Topic: [PATCH v3 17/18] eal: add function attributes for allocation functions Thread-Index: AdsShkrNgkFJrUY0STyQ9RInmn1hRAGzzIJQ References: <20240927204742.546164-1-stephen@networkplumber.org> <20240929154107.62539-1-stephen@networkplumber.org> <20240929154107.62539-18-stephen@networkplumber.org> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Stephen Hemminger" , Cc: "Tyler Retzlaff" , "Anatoly Burakov" 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: Sunday, 29 September 2024 17.35 >=20 > The allocation functions take a alignment argument that > can be useful to hint the compiler optimizer. >=20 > This is supported by Gcc and Clang but only useful with > Gcc because Clang gives warning if alignment is 0. This patch defines and uses __rte_alloc_align(). OK. >=20 > Recent 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(). This patch defines __rte_alloc_func(), but uses it in the next patch in = the series. Suggest either doing both here, or move the definition of = __rte_alloc_func() to the next patch. > +/** > + * Tells the compiler this is a function like malloc and that the > pointer > + * returned cannot alias any other pointer (ie new memory). There's a good example of its use here: https://developers.redhat.com/blog/2021/04/30/detecting-memory-management= -bugs-with-gcc-11-part-1-understanding-dynamic-allocation#detecting_misma= tched_deallocations It not only refers to memory, but also handle pointers. You might want to replace "ie new memory" by "ie new object" or similar. Please add the optional arguments to pass to __rte_alloc_func to the = macro description, e.g.: @param [free_func] The name of the deallocation function to free the allocated object @param [free_func_ptr_index] The deallocation function's argument index of the object pointer. PS: The brackets indicate that the parameter is optional. I didn't know, = so it is what I found on the internet. > + * > + * Also, with recent GCC versions also able to track that proper > + * dealloctor function is used for this pointer. > + */ > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >=3D 110000) > +#define __rte_alloc_func(...) \ > + __attribute__((malloc, malloc(__VA_ARGS__))) > + > +#elif defined(RTE_CC_GCC) || defined(RTE_CC_CLANG) > +#define __rte_alloc_func(...) \ > + __attribute__((malloc)) > +#else > +#define __rte_alloc_func(...) > +#endif The _func postfix seems superfluous. Macros hinting about Hot and cold = functions are simply __rte_hot and __rte_cold, without _func postfix. It's probably a matter of taste, so I'll leave it up to you. Minor detail: When looking at the code using the macro, it seems somewhat confusing = that the macro name is "__rte_alloc" when its arguments describe the = associated free function. But I have no ideas for a better name... Even if the two arguments were required, the primary purpose of the = macro is to inform the compiler that the function is an allocation = function; so that must be dominant in the name of the macro, which it is = with the current name. With the macro description updated, Series-Acked-by: Morten Br=F8rup