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 5984E43C0E; Tue, 27 Feb 2024 17:27:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 47D454027D; Tue, 27 Feb 2024 17:27:45 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 60C1340150 for ; Tue, 27 Feb 2024 17:27:44 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 1330BFE5C for ; Tue, 27 Feb 2024 17:27:44 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 07890FDD2; Tue, 27 Feb 2024 17:27:44 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.4 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.4 Received: from [192.168.1.59] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 34935FE5B; Tue, 27 Feb 2024 17:27:42 +0100 (CET) Message-ID: Date: Tue, 27 Feb 2024 17:27:41 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC v4 1/6] eal: add static per-lcore memory allocation facility To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org Cc: Stephen Hemminger References: <20240220084908.488252-2-mattias.ronnblom@ericsson.com> <20240225150330.517225-1-mattias.ronnblom@ericsson.com> <20240225150330.517225-2-mattias.ronnblom@ericsson.com> <98CBD80474FA8B44BF855DF32C47DC35E9F270@smartserver.smartshare.dk> <98CBD80474FA8B44BF855DF32C47DC35E9F278@smartserver.smartshare.dk> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F278@smartserver.smartshare.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 On 2024-02-27 16:05, Morten Brørup wrote: >> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se] >> Sent: Tuesday, 27 February 2024 14.44 >> >> On 2024-02-27 10:58, Morten Brørup wrote: >>>> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com] >>>> Sent: Sunday, 25 February 2024 16.03 >>> >>> [...] >>> >>>> +static void * >>>> +lcore_var_alloc(size_t size, size_t align) >>>> +{ >>>> + void *handle; >>>> + void *value; >>>> + >>>> + offset = RTE_ALIGN_CEIL(offset, align); >>>> + >>>> + if (offset + size > RTE_MAX_LCORE_VAR) { >>> >>> This would be the usual comparison: >>> if (lcore_buffer == NULL) { >>> >>>> + lcore_buffer = aligned_alloc(RTE_CACHE_LINE_SIZE, >>>> + LCORE_BUFFER_SIZE); >>>> + RTE_VERIFY(lcore_buffer != NULL); >>>> + >>>> + offset = 0; >>>> + } >>> >>> [...] >>> >>>> +/** >>>> + * Define a lcore variable handle. >>>> + * >>>> + * This macro defines a variable which is used as a handle to access >>>> + * the various per-lcore id instances of a per-lcore id variable. >>>> + * >>>> + * The aim with this macro is to make clear at the point of >>>> + * declaration that this is an lcore handler, rather than a regular >>>> + * pointer. >>>> + * >>>> + * Add @b static as a prefix in case the lcore variable are only to >> be >>>> + * accessed from a particular translation unit. >>>> + */ >>>> +#define RTE_LCORE_VAR_HANDLE(type, name) \ >>>> + RTE_LCORE_VAR_HANDLE_TYPE(type) name >>>> + >>> >>> The parameter is "name" here, and "handle" in other macros. >>> Just mentioning to make sure you thought about it. >>> >>>> +/** >>>> + * Get pointer to lcore variable instance with the specified lcore >> id. >>>> + */ >>>> +#define RTE_LCORE_VAR_LCORE_PTR(lcore_id, handle) \ >>>> + ((typeof(handle))__rte_lcore_var_lcore_ptr(lcore_id, handle)) >>>> + >>>> +/** >>>> + * Get value of a lcore variable instance of the specified lcore id. >>>> + */ >>>> +#define RTE_LCORE_VAR_LCORE_GET(lcore_id, handle) \ >>>> + (*(RTE_LCORE_VAR_LCORE_PTR(lcore_id, handle))) >>>> + >>>> +/** >>>> + * Set the value of a lcore variable instance of the specified lcore >> id. >>>> + */ >>>> +#define RTE_LCORE_VAR_LCORE_SET(lcore_id, handle, value) \ >>>> + (*(RTE_LCORE_VAR_LCORE_PTR(lcore_id, handle)) = (value)) >>> >>> I still think RTE_LCORE_VAR[_LCORE]_PTR() suffice, and >> RTE_LCORE_VAR[_LCORE]_GET/SET are superfluous. >>> But I don't insist on their removal. :-) >>> >> >> I'll remove them. One can always add them later. Nothing I've seen in >> the DPDK code base so far has been called for their use. >> >> Should the RTE_LCORE_VAR_PTR() be renamed RTE_LCORE_VAR_VALUE() (and >> still return a pointer, obviously)? "PTR" seems a little superfluous >> (Hungarian). "RTE_LCORE_VAR()" would be short, but not very descriptive. > > Good question... > > I would try to align this name and the name of the associated foreach macro, currently RTE_LCORE_VAR_FOREACH_VALUE(var, handle). > > It seems confusing to have a macro named _VALUE() returning a pointer. > (Which is why I also dislike the foreach macro's current name and "var" parameter name.) > Not sure I agree. In C, you often ask for a value and get a pointer to that value. I'll leave it VALUE() for now. > If it is supposed to be frequently used, a shorter name is preferable. > Which leans towards RTE_LCORE_VAR(). > > And then RTE_FOREACH_LCORE_VAR(iterator, handle) or RTE_LCORE_VAR_FOREACH(iterator, handle). > RTE_LCORE_VAR_FOREACH was the original name, which was changed because it was confusingly close to RTE_LCORE_FOREACH(), but had a different semantics in regards to which lcore ids are iterated over (EAL threads only, versus all lcore ids). > But then it is not obvious from the name that they operate on pointers. > We don't use Hungarian style in DPDK, so perhaps that is acceptable. > > > Your conclusion that GET/SET are not generally required inspired me for another idea... > Maybe returning a pointer is not the right thing to do! > > I wonder if there are any obstacles to generally dereferencing the lcore variable pointer, like this: > > #define RTE_LCORE_VAR_LCORE(lcore_id, handle) \ > (*(typeof(handle))__rte_lcore_var_lcore_ptr(lcore_id, handle)) > > It would work for both get and set: > RTE_LCORE_VAR(foo) = RTE_LCORE_VAR(bar); > > And also for functions being passed the address of the variable. > E.g. memset(&RTE_LCORE_VAR(foo), ...) would expand to: > memset(&(*(typeof(foo))__rte_lcore_var_lcore_ptr(rte_lcore_id(), foo)), ...); > > The value is usually accessed by means of a pointer, so no need to return *pointer. > One more thought, not related to the above discussion: > > The TLS per-lcore variables are built with "per_lcore_" prefix added to the names, like this: > #define RTE_DEFINE_PER_LCORE(type, name) \ > __thread __typeof__(type) per_lcore_##name > > Should the lcore variables have something similar, i.e.: > #define RTE_LCORE_VAR_HANDLE(type, name) \ > RTE_LCORE_VAR_HANDLE_TYPE(type) lcore_var_##name > I started out with a prefix, but I removed it, since you may want to access (copy, assign) the handler pointer directly, and thus need to know it's real name. Also, I didn't see why you need a prefix. For example, consider a section of code where you want to use one of two variables depending on condition. RTE_LCORE_VAR_HANDLE(actual, int); if (something) actual = some_handle; else actual = some_other_handle; int *value = RTE_LCORE_VAR_VALUE(actual); This above doesn't work if some_handle is actually named rte_lcore_var_some_handle or something like that. If you want to add a prefix (for which there shouldn't be a need), you would need a macro RTE_LCORE_VAR_NAME() as well, so the user can derive the actual name (including the prefix). > >> >>> With or without suggested changes... >>> >>> For the series, >>> Acked-by: Morten Brørup >>> >> >> Thanks for all help. > > Thank you for the detailed consideration of my feedback. >