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 480BE45E8B for ; Fri, 13 Dec 2024 07:58:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3ABC94064E; Fri, 13 Dec 2024 07:58:20 +0100 (CET) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id D3FF640263; Fri, 13 Dec 2024 07:58:17 +0100 (CET) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 999CD145F8; Fri, 13 Dec 2024 07:58:17 +0100 (CET) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 79FA6145F6; Fri, 13 Dec 2024 07:58:17 +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.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.85] (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 ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id DB96A14565; Fri, 13 Dec 2024 07:58:13 +0100 (CET) Message-ID: Date: Fri, 13 Dec 2024 07:58:13 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/3] power: defer lcore variable allocation To: David Marchand Cc: dev@dpdk.org, thomas@monjalon.net, frode.nordahl@canonical.com, mattias.ronnblom@ericsson.com, stable@dpdk.org, Anatoly Burakov , David Hunt , Sivaprasad Tummala , Stephen Hemminger , Chengwen Feng , Konstantin Ananyev , =?UTF-8?Q?Morten_Br=C3=B8rup?= References: <20241205175754.1673888-1-david.marchand@redhat.com> <20241205175754.1673888-3-david.marchand@redhat.com> <16f6d334-d999-4fe7-b177-6daf881b8f1e@lysator.liu.se> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On 2024-12-12 08:57, David Marchand wrote: > On Fri, Dec 6, 2024 at 12:29 PM Mattias Rönnblom wrote: >> I would wrap all RTE_LCORE_VAR_LCORE() and RTE_LCORE_VAR(). >> >> static struct pmd_core_cfg * >> get_cfg_lcore(unsigned int lcore_id) >> { >> assure_lcore_cfgs_alloced(); >> return RTE_LCORE_VAR_LCORE(lcore_cfgs, lcore_id); >> } >> >> static struct pmd_core_cfg * >> get_cfg(void) >> { >> get_cfg_lcore(rte_lcore_id()); >> } >> >> Add >> >> static void >> assure_lcore_cfgs_alloced(unsigned int lcore_id) >> { >> if (lcore_cfgs != NULL) > > == > Oops. >> lcore_cfgs_alloc(); >> } >> >> ..or maybe better merge assure_lcore_cfgs_alloced() and lcore_cfgs_alloc(). >> >> Makes it a little harder to make mistakes. > > clb_multiwait, clb_pause and clb_scale_freq callbacks can only be > reached after a successful call to > rte_power_ethdev_pmgmt_queue_enable. > Triggering an allocation in them means we are hiding a (internal) > programatic error as allocation and use of a lcore variable are > clearly separated atm. > If we keep the lcore var api as is, I would add an assert() (maybe > under a debug build option) in RTE_LCORE_VAR macros themselves, as > calling with a NULL handle means the initialisation path in some > code/RTE_LCORE_VAR API use was incorrect. > Sure, that would make sense. RTE_ASSERT(), that is. RTE_VERIFY() would be too expensive. > > Or because you propose to add the same type of helpers in both this > patch and the next, I am considering the other way: hide the > allocation in the RTE_LCORE_VAR* macros. > Checking for already allocated var in RTE_LCORE_VAR_ALLOC seems fine. > But the "fast path" RTE_LCORE_VAR would have an unwanted branch in most cases. > I would prefer to have the ALLOC() macro with semantics most people expect from a macro (or function) with that name, which is, I would argue, an unconditional allocation. It would make sense to have another macro, which performs an allocation only if the handle is NULL. RTE_LCORE_VAR_ASSURE_ALLOCATED(), or just RTE_LCORE_VAR_ASSURE() (although the latter sounds a little like an assertion, and not an allocation). RTE_LCORE_VAR_LAZY_ALLOC() I don't know. Something like that. > >> A somewhat unrelated question: why is pmd_core_cfg cache-line aligned? I >> don't think it should be. > > Before the conversion to per lcore variable, it was probably useful > (avoiding false sharing). > With the conversion, indeed, it looks like a waste of space. > It seems worth a separate fix. > > You will include it, or should I submit a separate patch?