Re: [PATCH v4] dts: Change hugepage runtime config to 2MB Exclusively

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

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:

<snip>

> 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
>