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 8E5DF43F03; Thu, 25 Apr 2024 10:00:46 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 17AAA436A5; Thu, 25 Apr 2024 10:00:46 +0200 (CEST) Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by mails.dpdk.org (Postfix) with ESMTP id 0F0E043663 for ; Thu, 25 Apr 2024 10:00:44 +0200 (CEST) Received: by mail-ej1-f47.google.com with SMTP id a640c23a62f3a-a55b2f49206so312792266b.1 for ; Thu, 25 Apr 2024 01:00:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1714032044; x=1714636844; darn=dpdk.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=NRkfq+hsrvsIrn7nRYBpFfu1yYeMYGbNHJU1PrAWlZY=; b=YV6T6Wr8mWYOKyp/urOzOvvweF7v0aG74xIe641wOAIcVoX8BHVkneE+MK+lgktbHI 6K5wGKlDrEZ77s5oG4leoitDSTnLBpBAAdTS+56XtSej8iZDUhs0gaHaSc329ZyI40WS 3qtxDATvrLjWHxrI962BjKPb6hGUbkQhP2i+lBxuyHBZwvdG8g0AGX3Hl5ajXZ1aTPeH wK/Sgi8rzkBQOndVHb3tI5YUE/+BJCS8/HI5S3bF0tR47LiUZPB+htumxvUZIB2MVBmw 7Df8byf65nq07xMUZPXCm5YaC63l9BV722utuKPd5YNF14UrOLMcRLOK2EBGiUHeBdP2 cpZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714032044; x=1714636844; h=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=NRkfq+hsrvsIrn7nRYBpFfu1yYeMYGbNHJU1PrAWlZY=; b=k3r2M1hzZ8SZ+zacVup1H7ugVPfWe8KkJnIyIIsP3BBSj4DsBneCtRorcRudJHpDPU Id9xWzIbyVBvieCu5AKdYF2QlOmiIjgUDSAbfAqomiSberMhbR0NStNpbPh7DlII8qG2 SBv/m2NvbTtVNV53+h8rVyvAfhgi9+VkHdiWD30VWXVQJrx5pzOPfnZauAdTo/SrrBUu bG8pGfrAkRrLm7zJ74k7Xuu7KsJwehf1ruz+KmmDPwJ6LaRNVZb5m/13naLexsZE26j1 /+l3CzAaABm1ur5K7YRDlvSFs8NzgrGQGKTlH0dyGmzftBylpc2Cjl7Stj9WnLwTkpkE P3aA== X-Forwarded-Encrypted: i=1; AJvYcCU8IdDpY9Z23OIqCd7zJtqQL7CTbNG02eKI0TIz6c3b9OcLPijuqSw9j+c2yV/L/CV4PXUotsqVnLoRClY= X-Gm-Message-State: AOJu0Ywz0cAyc1mdn/c9b+CFoYZOYVWHwmhAgllgewEPwbUjrqWNRZsJ 7qqbq2WhSUxYnt0p4EO3UarrIxjXqIWoohSghHvLEzhgzuRsso9/R71v0K9+4VwlqmhSuvMB2+n 8Ykv4Gv3Uap/zVsl0vvcF99DfTJey95nZyNB31Q== X-Google-Smtp-Source: AGHT+IEIU8OKvpmqv7J9NRx2DLZv8kOqJ7tPxq9Ldk5TDjhYwb6iKHtdNbcOM8+Yjnu0uTpIYa/CNFUqOPyyRUwMZe4= X-Received: by 2002:a17:906:17ca:b0:a52:119:3446 with SMTP id u10-20020a17090617ca00b00a5201193446mr2206409eje.34.1714032043732; Thu, 25 Apr 2024 01:00:43 -0700 (PDT) MIME-Version: 1.0 References: <20240404153106.19047-1-npratte@iol.unh.edu> <20240418161026.2839-1-npratte@iol.unh.edu> In-Reply-To: <20240418161026.2839-1-npratte@iol.unh.edu> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Thu, 25 Apr 2024 10:00:32 +0200 Message-ID: Subject: Re: [PATCH v4] dts: Change hugepage runtime config to 2MB Exclusively To: Nicholas Pratte Cc: jspewock@iol.unh.edu, wathsala.vithanage@arm.com, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, thomas@monjalon.net, mb@smartsharesystems.com, bruce.richardson@intel.com, yoan.picchi@foss.arm.com, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" 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 Just a few minor points, otherwise this looks good. > diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst > index 47b218b2c6..71473dbb3d 100644 > --- a/doc/guides/tools/dts.rst > +++ b/doc/guides/tools/dts.rst > @@ -131,7 +131,11 @@ There are two areas that need to be set up on a System Under Test: > > You may specify the optional hugepage configuration in the DTS config file. > If you do, DTS will take care of configuring hugepages, > - overwriting your current SUT hugepage configuration. > + overwriting your current SUT hugepage configuration. Configuration of hugepages via DTS > + allows only for allocation of 2MB hugepages, as doing so prevents accidental/over > + allocation of hugepages and hugepages sizes not recommended during runtime due to This wording is a bit confusing. What would make sense to me is "allocation of hugepages with hugepage sizes not recommended". Or am I missing something? > + contiguous memory space requirements. Thus, if you require hugepage > + sizes not equal to 2MB, then this configuration must be done outside of the DTS framework. > > * System under test configuration > > diff --git a/dts/conf.yaml b/dts/conf.yaml > index 8068345dd5..56c3ae6f4c 100644 > --- a/dts/conf.yaml > +++ b/dts/conf.yaml > @@ -35,7 +35,7 @@ nodes: > lcores: "" # use all the available logical cores > use_first_core: false # tells DPDK to use any physical core > memory_channels: 4 # tells DPDK to use 4 memory channels > - hugepages: # optional; if removed, will use system hugepage configuration > + hugepages_2mb: # optional; if removed, will use system hugepage configuration > amount: 256 I noticed this mistake I made a while back, but this looks like a good opportunity to point it out. Amount is used with uncountable nouns and hugepages is countable. We should correct this mistake (rename to number in all places, I think that's used somewhere in Linux) and this patch could be a good place to do it. Or maybe a separate one or even a separate patchset. What do you think, would you please fix this while you're making hugepage changes? > force_first_numa: false > ports: > @@ -71,7 +71,7 @@ nodes: > os_driver: rdma > peer_node: "SUT 1" > peer_pci: "0000:00:08.1" > - hugepages: # optional; if removed, will use system hugepage configuration > + hugepages_2mb: # optional; if removed, will use system hugepage configuration > amount: 256 > force_first_numa: false > traffic_generator: > diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py > index 74061f6262..056a031ca0 100644 > --- a/dts/framework/testbed_model/node.py > +++ b/dts/framework/testbed_model/node.py > @@ -97,6 +97,10 @@ def __init__(self, node_config: NodeConfiguration): > self.virtual_devices = [] > self._init_ports() > > + @property > + def _hugepage_default_size(self) -> int: > + return 2048 > + I'm thinking about the placement of this and I'm kinda torn between Node and OSSession. In Node, it makes sense as it basically means "no matter what sort of node we're connected to, we're going to use this hugepage size". In OSSession, it would make more sense if we need a different hugepage size based on the OS (and possibly arch, which could be passed to it). For now, leaving it in Node is probably better. We can always move it if need be. But I don't really get why it's a property. Using properties doesn't really make sense if we're not putting any logic into it. This could just as easily be self._hugepage_default_size = 2048 in __init__(). > def _init_ports(self) -> None: > self.ports = [Port(self.name, port_config) for port_config in self.config.ports] > self.main_session.update_ports(self.ports) > @@ -266,7 +270,9 @@ def _setup_hugepages(self) -> None: > """ > if self.config.hugepages: > self.main_session.setup_hugepages( > - self.config.hugepages.amount, self.config.hugepages.force_first_numa > + self.config.hugepages.amount, > + self._hugepage_default_size, > + self.config.hugepages.force_first_numa, > ) > > def configure_port_state(self, port: Port, enable: bool = True) -> None: > diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py > index d5bf7e0401..5d58400cbe 100644 > --- a/dts/framework/testbed_model/os_session.py > +++ b/dts/framework/testbed_model/os_session.py > @@ -345,7 +345,9 @@ def get_dpdk_file_prefix(self, dpdk_prefix: str) -> str: > """ > > @abstractmethod > - def setup_hugepages(self, hugepage_count: int, force_first_numa: bool) -> None: > + def setup_hugepages( > + self, hugepage_count: int, hugepage_size: int, force_first_numa: bool > + ) -> None: > """Configure hugepages on the node. > > Get the node's Hugepage Size, configure the specified count of hugepages > @@ -353,6 +355,7 @@ def setup_hugepages(self, hugepage_count: int, force_first_numa: bool) -> None: > > Args: > hugepage_count: Configure this many hugepages. > + hugepage_size: Configure hugepages of this size (currently not used in the config) The dot at the end of the sentence is missing. The reminder in the parentheses is not needed, the OSSession class is decoupled from the rest of the classes (and is thus not aware of the config). > force_first_numa: If :data:`True`, configure hugepages just on the first numa node. > """ > > -- > 2.44.0 >