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 2B46B45EBC for ; Mon, 16 Dec 2024 11:03:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 217CC40144; Mon, 16 Dec 2024 11:03:07 +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 DBAA840144 for ; Mon, 16 Dec 2024 11:03:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1734343385; 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=Qakf0Q6i5rpJw0bHZ/QjSC70J5GBKn5/22MeUYsP3lH25nSYS0ZuxTw/NpFsdfzjh85tAf Eq0Z5bop8tO1mUKIuPZIPB+1wb6q2Y7qZKTJvks5g01gPlaBauGgnfwPJ5WPBtDV9OnSqE 9iD9M0CyoiMjrPj3vMA0+oKW0jy5C1w= Received: from mail-lf1-f70.google.com (mail-lf1-f70.google.com [209.85.167.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-447-dptexSv3P3CfFwW3MXVdaw-1; Mon, 16 Dec 2024 05:03:04 -0500 X-MC-Unique: dptexSv3P3CfFwW3MXVdaw-1 X-Mimecast-MFC-AGG-ID: dptexSv3P3CfFwW3MXVdaw Received: by mail-lf1-f70.google.com with SMTP id 2adb3069b0e04-53e396834daso323608e87.2 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=UQk9FYzCCO4aMYER7Zhjsi+wAkteK4BAacCV6UGo5Q+uOpiXe9uNHAbj4ZIo24GExF aoM+4PODQ7zIcAJs9BbrpgdZqcQ/r9O00DrGG8LzL3eOvWiJy5J0YSoa66EKSokZ1b3f uFy6AXeVDiIJY98gdfOuUy+ZZKLiwvWMskIjd6x/Rgknwg1ww4VUGDyPMAmGufZUcWlj lbxxjpuBQu2y/9Z0M4Zuf7ys+Hd1pBRlgEamc5dzXDmsOLa8yY+yU09lrdnqQG0aAlHR DiA+5jbXJwbSDC6gPqOMQVp8z7q/y+ihpCwfJMEHZNAYMmo/5D2UjTinaDGEPrKTR0/O eosg== X-Forwarded-Encrypted: i=1; AJvYcCWqCJFXrsGCv5g5K8JblORPW1KYhRrU9f1cU/1V1g5/SLX5W0OnbOPEGDVbBGTzvHC+mylKhNA=@dpdk.org X-Gm-Message-State: AOJu0YwSSCcYLnRRX6zJlUNszxZ0oUDH73irvCW1M/xqNinyTkjKUeDm 5LBESQpUzzUKNahZnpNDKKsgG5VgOuGfL/dziGm8i4XrQtUzJ3JGid7820iM9AuGkvL/NZuU3AX PzJXYxtbH8wuuGG3hK7MM87VxUkN+302UxNcVKOABdg9Yk+QeqrgwZ6qJhECxn/coyFQZGBg98Q EanYS94BLEoBk3jyiOTK4= X-Gm-Gg: ASbGncvFpFsEFfHT/MpjJEJh06sYgzBR5wzTEAyjsb+sbgaD19hwtd+I5hjqWzu7Taz pzbit76nI6dAQyjV2MK6S+XYzmB0Qo6s+jpjBXmym X-Received: by 2002:a2e:b604:0:b0:302:251a:bd04 with SMTP id 38308e7fff4ca-30254438172mr31127461fa.9.1734343382879; 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: ADPv-Lv4mG_JBd52lqQ3G52gQ6N5vkJR69W2vfhOsCQ_1734343383 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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