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 30DB2454AA; Thu, 20 Jun 2024 15:41:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B79FD40612; Thu, 20 Jun 2024 15:41:30 +0200 (CEST) Received: from mail-lj1-f180.google.com (mail-lj1-f180.google.com [209.85.208.180]) by mails.dpdk.org (Postfix) with ESMTP id EC91C402D2 for ; Thu, 20 Jun 2024 15:41:29 +0200 (CEST) Received: by mail-lj1-f180.google.com with SMTP id 38308e7fff4ca-2ebd590a8b9so627911fa.3 for ; Thu, 20 Jun 2024 06:41:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1718890889; x=1719495689; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=RrhjCMIegOEDR71JldB7aPHcrXZ9w/N5WyDq79gtpBA=; b=h7z/CwVc7jMv44FkE+qQRqHPL4G4OqH870fIJ4i445sYC2uNtv8YLLaV64sjiuBpG+ KKFwjFN+v3opLuewcE5F4IrRrtcvukZDGOHFg183MQRDUmh9DKgCDEpidAQgiJD7YYlj vJaNz5HmZbzp/deq4/V/gVgB1TIKR+U5DDEP0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718890889; x=1719495689; 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=RrhjCMIegOEDR71JldB7aPHcrXZ9w/N5WyDq79gtpBA=; b=Sq6QoKno1fBXah1p1BOjfJaZ4b7rTIgEsP2MKYuUUafkQ4yMblr6WQyv6zDhbyrA3W J0LZ38ZzfVCxRIwZFRRV5FyYy2QUo5m8NbLfol8eewXVNODQYWYPqDvCJBTV2SThTNzu Yh46CfGCKNXzfl8zU4+uWdLrlD/YeYyNpUbChfG/HutzBYZQSUoc/I6PrB7jq3DE4a6U jJuQQWoeR/YJ6aSHPH0yTV7e5uGpF1NNe5GKq3lh/1Na/AqQbuHwCw9edetY89eQE4C7 9K0EseWkFg8qE9MKyT4H2WgjERqRW9p9dn6nvvVPhTTcgAkVODqJdQOV4ZMG22xGVL/j Sh8g== X-Forwarded-Encrypted: i=1; AJvYcCXYZmu3FD6EZe3ojhq0MUq0cw5agOEhyLykPWjg2KJKXHZ7EUCJWhl6HQ0HoUeuSjQi2sRf2OIcxkjqo1c= X-Gm-Message-State: AOJu0Ywn5COXM4ZMBiE8Sckc2yEWTlnteInqbjsT3OawGCtEqyEtE7ux BHgGyBqxRwGddFav+5vjtvI30WhX6Ihh3BiDS8pF3+QCkru0cEeqSAcZX/02tmiTNwGcNe4kWdq aKLhHGjD9t15W2F7zMWQiayXKOiizpTbN7PNXDw== X-Google-Smtp-Source: AGHT+IFEUpS+yOGQwIX2Q2nnEakSO2+YSGqMw2FgJ154Oi04i+XC8/I2AZEv5ZbcVwFGrSLRFS4eiTkbEYLiYhOf8SE= X-Received: by 2002:a2e:9252:0:b0:2ec:3e02:9734 with SMTP id 38308e7fff4ca-2ec3e0297a4mr26479591fa.2.1718890889251; Thu, 20 Jun 2024 06:41:29 -0700 (PDT) MIME-Version: 1.0 References: <20240613201831.9748-3-npratte@iol.unh.edu> <20240613201831.9748-7-npratte@iol.unh.edu> In-Reply-To: From: Nicholas Pratte Date: Thu, 20 Jun 2024 09:41:18 -0400 Message-ID: Subject: Re: [PATCH 2/4] dts: Use First Core Logic Change To: Jeremy Spewock Cc: Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, luca.vizzarro@arm.com, juraj.linkes@pantheon.tech, bruce.richardson@intel.com, probb@iol.unh.edu, dmarx@iol.unh.edu, yoan.picchi@foss.arm.com, dev@dpdk.org 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, Jun 14, 2024 at 2:09=E2=80=AFPM Jeremy Spewock wrote: > > On Thu, Jun 13, 2024 at 4:21=E2=80=AFPM Nicholas Pratte wrote: > > > > Removed use_first_core from the conf.yaml in favor of determining this > > within the framework. use_first_core continue to serve a purpose in tha= t > > it is only enabled when core 0 is explicitly provided in the > > configuration. Any other configuration, including "" or "any," will > > omit core 0. > > > > Documentation reworks are included to reflect the changes made. > > > > Bugzilla ID: 1360 > > Signed-off-by: Nicholas Pratte > > > > --- > > doc/guides/tools/dts.rst | 9 +++------ > > dts/conf.yaml | 3 +-- > > dts/framework/config/__init__.py | 11 +++++++---- > > dts/framework/config/conf_yaml_schema.json | 6 +----- > > dts/framework/testbed_model/node.py | 9 +++++++++ > > 5 files changed, 21 insertions(+), 17 deletions(-) > > > > diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst > > index da85295db9..fbb5c6f17b 100644 > > --- a/doc/guides/tools/dts.rst > > +++ b/doc/guides/tools/dts.rst > > @@ -546,15 +546,12 @@ involved in the testing. These can be defined wit= h the following mappings: > > +-----------------------+------------------------------------------= ---------------------------------------------+ > > | ``os`` | The operating system of this node. See `O= S`_ for supported values. | > > +-----------------------+------------------------------------------= ---------------------------------------------+ > > - | ``lcores`` | | (*optional*, defaults to 1) *string* = =E2=80=93 Comma-separated list of logical | > > - | | | cores to use. An empty string means use= all lcores. | > > + | ``lcores`` | | (*optional*, defaults to 1 if not used)= *string* =E2=80=93 Comma-separated list of logical | > > I think just leaving this as "defaults to 1" is fine here. It's more > explicit to say "if it isn't used", but just saying it defaults I > think implies that enough. Good point, and you're right. '*optional* and 'if not used' are redundant. > > > + | | | cores to use. An empty string means use= all lcores except core 0. core 0 is used | > > + | | | only when explicitly specified = | > > | | = | > > | | **Example**: ``1,2,3,4,5,18-22`` = | > > +-----------------------+------------------------------------------= ---------------------------------------------+ > > - | ``use_first_core`` | (*optional*, defaults to ``false``) *bool= ean* | > > - | | = | > > - | | Indicates whether DPDK should use only th= e first physical core or not. | > > - +-----------------------+------------------------------------------= ---------------------------------------------+ > > | ``memory_channels`` | (*optional*, defaults to 1) *integer* = | > > | | = | > > | | The number of the memory channels to use.= | > > > diff --git a/dts/framework/config/__init__.py b/dts/framework/config/__= init__.py > > index 5a127a1207..6bc290a56a 100644 > > --- a/dts/framework/config/__init__.py > > +++ b/dts/framework/config/__init__.py > > @@ -245,6 +245,9 @@ def from_dict( > > hugepage_config_dict["force_first_numa"] =3D False > > hugepage_config =3D HugepageConfiguration(**hugepage_confi= g_dict) > > > > + lcores =3D "1" if "lcores" not in d else d["lcores"] if "any" = not in d["lcores"] else "" > > + use_first_core =3D "0" in lcores > > + > > # The calls here contain duplicated code which is here because= Mypy doesn't > > # properly support dictionary unpacking with TypedDicts > > if "traffic_generator" in d: > > @@ -255,8 +258,8 @@ def from_dict( > > password=3Dd.get("password"), > > arch=3DArchitecture(d["arch"]), > > os=3DOS(d["os"]), > > - lcores=3Dd.get("lcores", "1"), > > - use_first_core=3Dd.get("use_first_core", False), > > + lcores=3Dlcores, > > + use_first_core=3Duse_first_core, > > I wonder if we could completely remove the "use_first_core" attribute > from the config classes. It seems like it's only used when you're > getting remote CPUs to skip core 0 based on the condition. With these > new changes it seems like we just assume that if 0 is in the list, we > will use it, otherwise we simply won't, so I don't think we need this > condition to detect whether we should skip or not anymore, do we? It's an interesting point, and it's one that I honestly thought about right after I completed the last patch in this series (DPDK_config config attribute). It wasn't until I tried implementing the 'dpdk_config' attribute that I realized the broader scope of lcore filtering in the framework. The framework is currently filtering a list of lcores in two spots of the framework. The last patch, as you saw, addresses the aforementioned problem by removing 'use_first_core' from the framework completely. In retrospect, I could have dropped this commit and just included the 'dpdk_config' patch with the 'use_first_core' changes included, but I suppose it's not bad for others to see both implementations and discuss what the better option is. > > > hugepages=3Dhugepage_config, > > ports=3D[PortConfig.from_dict(d["name"], port) for por= t in d["ports"]], > > traffic_generator=3DTrafficGeneratorConfig.from_dict(d= ["traffic_generator"]), > > @@ -269,8 +272,8 @@ def from_dict( > > password=3Dd.get("password"), > > arch=3DArchitecture(d["arch"]), > > os=3DOS(d["os"]), > > - lcores=3Dd.get("lcores", "1"), > > - use_first_core=3Dd.get("use_first_core", False), > > + lcores=3Dlcores, > > + use_first_core=3Duse_first_core, > > hugepages=3Dhugepage_config, > > ports=3D[PortConfig.from_dict(d["name"], port) for por= t in d["ports"]], > > memory_channels=3Dd.get("memory_channels", 1), > > > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbe= d_model/node.py > > index 74061f6262..470cd18e30 100644 > > --- a/dts/framework/testbed_model/node.py > > +++ b/dts/framework/testbed_model/node.py > > @@ -93,6 +93,15 @@ def __init__(self, node_config: NodeConfiguration): > > self.lcores, LogicalCoreList(self.config.lcores) > > ).filter() > > > > + if LogicalCore(lcore=3D0, core=3D0, socket=3D0, node=3D0) in s= elf.lcores: > > This condition fits if you completely remove the `use_first_core` > boolean from the NodeConfiguration, but if we decide not to I think it > could be shortened to just: > > if config.use_first_core: Noted, and good catch! I'll keep this in mind depending on which way the wind blows.