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 C5FC243BA0; Fri, 23 Feb 2024 11:19:09 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 50D214068A; Fri, 23 Feb 2024 11:19:09 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 65EE9402ED for ; Fri, 23 Feb 2024 11:19:07 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 132271DC76 for ; Fri, 23 Feb 2024 11:19:07 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 06E5F1DD0F; Fri, 23 Feb 2024 11:19:07 +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 B3BF51DD8E; Fri, 23 Feb 2024 11:19:05 +0100 (CET) Message-ID: Date: Fri, 23 Feb 2024 11:19:05 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC v3 5/6] service: keep per-lcore state in lcore variable Content-Language: en-US To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org Cc: Stephen Hemminger References: <20240219094036.485727-2-mattias.ronnblom@ericsson.com> <20240220084908.488252-1-mattias.ronnblom@ericsson.com> <20240220084908.488252-6-mattias.ronnblom@ericsson.com> <98CBD80474FA8B44BF855DF32C47DC35E9F24A@smartserver.smartshare.dk> From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F24A@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-22 10:42, Morten Brørup wrote: >> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com] >> Sent: Tuesday, 20 February 2024 09.49 >> >> Replace static array of cache-aligned structs with an lcore variable, >> to slightly benefit code simplicity and performance. >> >> Signed-off-by: Mattias Rönnblom >> --- > > >> @@ -486,8 +489,7 @@ service_runner_func(void *arg) >> { >> RTE_SET_USED(arg); >> uint8_t i; >> - const int lcore = rte_lcore_id(); >> - struct core_state *cs = &lcore_states[lcore]; >> + struct core_state *cs = RTE_LCORE_VAR_PTR(lcore_states); > > Typo: TAB -> SPACE. > Will fix. >> >> rte_atomic_store_explicit(&cs->thread_active, 1, >> rte_memory_order_seq_cst); >> >> @@ -533,13 +535,16 @@ service_runner_func(void *arg) >> int32_t >> rte_service_lcore_may_be_active(uint32_t lcore) >> { >> - if (lcore >= RTE_MAX_LCORE || !lcore_states[lcore].is_service_core) >> + struct core_state *cs = >> + RTE_LCORE_VAR_LCORE_PTR(lcore, lcore_states); >> + >> + if (lcore >= RTE_MAX_LCORE || !cs->is_service_core) >> return -EINVAL; > > This comment is mostly related to patch 1 in the series... > > You are setting cs = RTE_LCORE_VAR_LCORE_PTR(lcore, ...) before validating that lcore < RTE_MAX_LCORE. I wondered if that potentially was an overrun bug. > > It is obvious when looking at the RTE_LCORE_VAR_LCORE_PTR() macro implementation, but perhaps its description could mention that it is safe to use with an "invalid" lcore_id, but not dereferencing it. > I thought about adding something equivalent to an RTE_ASSERT() on lcore_id in the dereferencing macros, but then I thought that maybe it is a valid use case to pass invalid lcore ids. Invalid ids being OK or not, I think the above code should do "cs = /../" *after* the lcore id check. Now it looks strange and force the reader to consider if this is valid or not, for no good reason. The lcore variable API docs should probably explicitly allow invalid core id in the macros.