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 991C8459C3; Wed, 18 Sep 2024 09:00:40 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5BDED4069F; Wed, 18 Sep 2024 09:00:40 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id F2B674029B for ; Wed, 18 Sep 2024 09:00:38 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id AA77B1D7B9 for ; Wed, 18 Sep 2024 09:00:38 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 9E2601D6DA; Wed, 18 Sep 2024 09:00:38 +0200 (CEST) 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.2 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.2 Received: from [192.168.1.86] (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 ECFF51D7B8; Wed, 18 Sep 2024 09:00:36 +0200 (CEST) Message-ID: <840f3c05-609e-466b-a23d-282ff8289cca@lysator.liu.se> Date: Wed, 18 Sep 2024 09:00:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v4 1/7] eal: add static per-lcore memory allocation facility To: Konstantin Ananyev , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , "dev@dpdk.org" Cc: =?UTF-8?Q?Morten_Br=C3=B8rup?= , Stephen Hemminger , Konstantin Ananyev , David Marchand , Jerin Jacob References: <20240912084429.703405-2-mattias.ronnblom@ericsson.com> <20240916105210.721315-1-mattias.ronnblom@ericsson.com> <20240916105210.721315-2-mattias.ronnblom@ericsson.com> <23d1b3c5828546128b9ed21e98ac1ae1@huawei.com> <78410500adf8482c97b15ae72b635df0@huawei.com> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <78410500adf8482c97b15ae72b635df0@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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-09-17 18:11, Konstantin Ananyev wrote: >>>> + >>>> +/** >>>> + * Get pointer to lcore variable instance with the specified lcore id. >>>> + * >>>> + * @param lcore_id >>>> + * The lcore id specifying which of the @c RTE_MAX_LCORE value >>>> + * instances should be accessed. The lcore id need not be valid >>>> + * (e.g., may be @ref LCORE_ID_ANY), but in such a case, the pointer >>>> + * is also not valid (and thus should not be dereferenced). >>>> + * @param handle >>>> + * The lcore variable handle. >>>> + */ >>>> +#define RTE_LCORE_VAR_LCORE_VALUE(lcore_id, handle) \ >>>> + ((typeof(handle))rte_lcore_var_lcore_ptr(lcore_id, handle)) >>>> + >>>> +/** >>>> + * Get pointer to lcore variable instance of the current thread. >>>> + * >>>> + * May only be used by EAL threads and registered non-EAL threads. >>>> + */ >>>> +#define RTE_LCORE_VAR_VALUE(handle) \ >>>> + RTE_LCORE_VAR_LCORE_VALUE(rte_lcore_id(), handle) >>> >>> Would it make sense to check that rte_lcore_id() != LCORE_ID_ANY? >>> After all if people do not want this extra check, they can probably use >>> RTE_LCORE_VAR_LCORE_VALUE(rte_lcore_id(), handle) >>> explicitly. >>> >> >> It would make sense, if it was an RTE_ASSERT(). Otherwise, I don't think >> so. Attempting to gracefully handle API violations is bad practice, imo. > > Ok, RTE_ASSERT() might be a good compromise. > As I said in another mail for that thread, I wouldn't insist here. > After a having a closer look at this issue, I'm not so sure any more. Such an assertion would disallow the use of the macros to retrieve a potentially-invalid pointer, which is then never used, in case it is invalid. >> >>>> + >>>> +/** >>>> + * Iterate over each lcore id's value for an lcore variable. >>>> + * >>>> + * @param value >>>> + * A pointer successively set to point to lcore variable value >>>> + * corresponding to every lcore id (up to @c RTE_MAX_LCORE). >>>> + * @param handle >>>> + * The lcore variable handle. >>>> + */ >>>> +#define RTE_LCORE_VAR_FOREACH_VALUE(value, handle) \ >>>> + for (unsigned int lcore_id = \ >>>> + (((value) = RTE_LCORE_VAR_LCORE_VALUE(0, handle)), 0); \ >>>> + lcore_id < RTE_MAX_LCORE; \ >>>> + lcore_id++, (value) = RTE_LCORE_VAR_LCORE_VALUE(lcore_id, handle)) >>> >>> Might be a bit better (and safer) to make lcore_id a macro parameter? >>> I.E.: >>> define RTE_LCORE_VAR_FOREACH_VALUE(value, handle, lcore_id) \ >>> for ((lcore_id) = ... >>> >> >> Why? > > Variable with the same name (and type) can be defined by user before the loop, > With the intention to use it inside the loop. > Just like it happens here (in patch #2): > + unsigned int lcore_id; > ..... > + /* take the opportunity to test the foreach macro */ > + int *v; > + lcore_id = 0; > + RTE_LCORE_VAR_FOREACH_VALUE(v, test_int) { > + TEST_ASSERT_EQUAL(states[lcore_id].new_value, *v, > + "Unexpected value on lcore %d during " > + "iteration", lcore_id); > + lcore_id++; > + } > + > > Indeed. I'll change it. I suppose you could also have issues if you nested the macro, although those could be solved by using something like __COUNTER__ to create a unique name. Supplying the variable name does defeat part of the purpose of the RTE_LCORE_VAR_FOREACH_VALUE. > >