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 841B845EBC; Mon, 16 Dec 2024 11:03:09 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 65B02402B4; Mon, 16 Dec 2024 11:03:09 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 0C4BD402D0 for ; Mon, 16 Dec 2024 11:03:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1734343387; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3s34NFGHjByUoWMyLU/JoYIXEroF0HRiE/e/USBWMf0=; b=i9ykyTnchfWfz3mLSxvQ1R2nD/ul9FHh/06n7hqIefVBDQ1UJgTWush7PSOAAxqJC4y31C DdXo20GNX9En67ibDchrgMMvLts+a/r/MCGFw3h80WzsI7cIWcZVAbd/IX+zYCBXiAa8v6 eoNJSMmj6AbuaiCTkDIqGGN8XB0oNpY= Received: from mail-lj1-f198.google.com (mail-lj1-f198.google.com [209.85.208.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-447-DFigy2JqNKi8wFsW2V2lqQ-1; Mon, 16 Dec 2024 05:03:04 -0500 X-MC-Unique: DFigy2JqNKi8wFsW2V2lqQ-1 X-Mimecast-MFC-AGG-ID: DFigy2JqNKi8wFsW2V2lqQ Received: by mail-lj1-f198.google.com with SMTP id 38308e7fff4ca-3023f0f1852so18110291fa.1 for ; Mon, 16 Dec 2024 02:03:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1734343383; x=1734948183; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3s34NFGHjByUoWMyLU/JoYIXEroF0HRiE/e/USBWMf0=; b=oq2Opt4lYSBVvAzguu1wWReAVPVsK2KzLDkd4PJM2wqmf+M0U1YVzZGCE8looBy29D umamIjEZobamTjDQ/I65LJ5kDXIP2dciV7heorMV8roGJOK53H3B/xglVqQTYYhtre2k mNwGRAaz/sl2gPZerxduRgWEU5sdBrP/ptRlLE5s/7cF+yotFysp+3itwKPTtCQbIXNa ALTVzXH1xfPKKl1U/A59sv27pZkcfhLBw9GpOkX2D0/WuDcZ8XLCSwQVUH+FU6EEmrid bRptPZJ4OHBD2X3XTOPT0HZwjiOO7t3Co/cCTjxtrV8jvsOjJJwYe0p6uSPKp2fPDVq0 bQ9Q== X-Gm-Message-State: AOJu0YzJHt0Nvi1y8es1G89jBuHNDto9EBLIapXEuqpkknNoNw9GBXM+ p+J/Uw0VZ9CMWwMiDPQmRu/9cbLzrN9vJwhNvqlsoYM+kDvCDBov7APtDoYA8mgWtO002euWFpV 77kw3WabLhaHBCK857U8x0tkXDw4vRkeLhY3hLFV8OkZctOrU5476orD9wh922JJ3LF4Qc6Oevv THT9r1BX6alwl8aUM= X-Gm-Gg: ASbGncugtDqNsrICgmzTGT/1aybzrkzZK13pCiAX/8p6v3N/ba+hqHAVMEvDrzmssUb Djc1XBU7Z50M5qb+U5Shukr/Bj/fL1rWDjbaRsM5s X-Received: by 2002:a2e:b604:0:b0:302:251a:bd04 with SMTP id 38308e7fff4ca-30254438172mr31127541fa.9.1734343382883; Mon, 16 Dec 2024 02:03:02 -0800 (PST) X-Google-Smtp-Source: AGHT+IFZacWaM9PJDMFkq25haXSVhkUSFLUQWq+R+6B60FjeUvXLQnBizzlpJEmPNwiMVKc9EkaEPw6AJttNgUjrfNo= X-Received: by 2002:a2e:b604:0:b0:302:251a:bd04 with SMTP id 38308e7fff4ca-30254438172mr31127391fa.9.1734343382471; Mon, 16 Dec 2024 02:03:02 -0800 (PST) MIME-Version: 1.0 References: <20241205175754.1673888-1-david.marchand@redhat.com> <20241205175754.1673888-3-david.marchand@redhat.com> <16f6d334-d999-4fe7-b177-6daf881b8f1e@lysator.liu.se> In-Reply-To: From: David Marchand Date: Mon, 16 Dec 2024 11:02:51 +0100 Message-ID: Subject: Re: [PATCH 2/3] power: defer lcore variable allocation To: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= 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?= X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: oxMnSjkWx0mJGdO2m6wD8NcMXOxaXUjuYIXfqQiuveE_1734343383 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Fri, Dec 13, 2024 at 7:58=E2=80=AFAM Mattias R=C3=B6nnblom wrote: > On 2024-12-12 08:57, David Marchand wrote: > > 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. Yes, I'll send in next revision. > > > > > 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. - In the power libary case, allocating the lcore variable is followed by the initialisation of the lcore variable internals (per lcore tailqs). For this patch, I will rename the alloc_lcore_cfgs helper I had in v1 as: static void +init_lcore_cfgs(void) +{ + struct pmd_core_cfg *lcore_cfg; + unsigned int lcore_id; + + if (lcore_cfgs !=3D NULL) + return; + + RTE_LCORE_VAR_ALLOC(lcore_cfgs); + + /* initialize all tailqs */ + RTE_LCORE_VAR_FOREACH(lcore_id, lcore_cfg, lcore_cfgs) + TAILQ_INIT(&lcore_cfg->head); +} and only keep those checks in the public symbols. - About more macros, I am wondering if this is needed in the end. Adding assertions in the lcore var accessor should catch incorrect initialisation path. > > > >> 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? I'll send it in next revision. --=20 David Marchand