[v5,2/2] dts: Change hugepage 'amount' to a different term
Checks
Commit Message
The term 'amount' is used for uncountable nouns. Since total hugepages
is a discrete value (i.e. countable), the declaration of the 'amount'
key value pair should be changes to a different term in both the config
and the rest of the code.
Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
---
dts/conf.yaml | 4 ++--
dts/framework/config/__init__.py | 4 ++--
dts/framework/config/conf_yaml_schema.json | 6 +++---
dts/framework/config/types.py | 2 +-
dts/framework/testbed_model/linux_session.py | 4 ++--
dts/framework/testbed_model/node.py | 2 +-
6 files changed, 11 insertions(+), 11 deletions(-)
Comments
On Tue, Apr 30, 2024 at 8:47 PM Nicholas Pratte <npratte@iol.unh.edu> wrote:
>
> The term 'amount' is used for uncountable nouns. Since total hugepages
> is a discrete value (i.e. countable), the declaration of the 'amount'
> key value pair should be changes to a different term in both the config
> and the rest of the code.
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
Reviewed-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
Reviewed-by: Luca Vizzarro <luca.vizzarro@arm.com>
On Tue, Apr 30, 2024 at 02:45:33PM -0400, Nicholas Pratte wrote:
> The term 'amount' is used for uncountable nouns. Since total hugepages
> is a discrete value (i.e. countable), the declaration of the 'amount'
> key value pair should be changes to a different term in both the config
> and the rest of the code.
>
> Signed-off-by: Nicholas Pratte <npratte@iol.unh.edu>
> ---
> dts/conf.yaml | 4 ++--
> dts/framework/config/__init__.py | 4 ++--
> dts/framework/config/conf_yaml_schema.json | 6 +++---
> dts/framework/config/types.py | 2 +-
> dts/framework/testbed_model/linux_session.py | 4 ++--
> dts/framework/testbed_model/node.py | 2 +-
> 6 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/dts/conf.yaml b/dts/conf.yaml
> index 56c3ae6f4c..44b5e4ec84 100644
> --- a/dts/conf.yaml
> +++ b/dts/conf.yaml
> @@ -36,7 +36,7 @@ nodes:
> use_first_core: false # tells DPDK to use any physical core
> memory_channels: 4 # tells DPDK to use 4 memory channels
> hugepages_2mb: # optional; if removed, will use system hugepage configuration
> - amount: 256
> + quantity: 256
> force_first_numa: false
Sorry to be late to the reviews here, but since this is a countable value -
as you state in the cover letter- would "number" or "count" not be better
terms. To me, "quantity" is just a synonym of "amount", and can be used for
uncountable values too, e.g. "a quantity of water".
/Bruce
On 07/05/2024 13:05, Bruce Richardson wrote:
> Sorry to be late to the reviews here, but since this is a countable value -
> as you state in the cover letter- would "number" or "count" not be better
> terms. To me, "quantity" is just a synonym of "amount", and can be used for
> uncountable values too, e.g. "a quantity of water".
Hi Bruce,
The change is based on the readability and intuitiveness of the
configuration file. In which case "number" could be ambiguous:
hugepages_2mb:
number: 100
And here I could see "count" working:
hugepages_2mb:
count: 100
But since the change is propagated for consistency. "count" would no
longer be well fitting in the rest:
"description": "The count of hugepages to configure. Hugepage
size will be the system default."
It seems that "quantity" may be the best fitting here while retaining
naming consistency.
Best,
Luca
On Tue, May 07, 2024 at 01:43:30PM +0100, Luca Vizzarro wrote:
> On 07/05/2024 13:05, Bruce Richardson wrote:
> > Sorry to be late to the reviews here, but since this is a countable value -
> > as you state in the cover letter- would "number" or "count" not be better
> > terms. To me, "quantity" is just a synonym of "amount", and can be used for
> > uncountable values too, e.g. "a quantity of water".
>
>
> Hi Bruce,
>
> The change is based on the readability and intuitiveness of the
> configuration file. In which case "number" could be ambiguous:
>
> hugepages_2mb:
> number: 100
>
> And here I could see "count" working:
>
> hugepages_2mb:
> count: 100
>
> But since the change is propagated for consistency. "count" would no longer
> be well fitting in the rest:
>
> "description": "The count of hugepages to configure. Hugepage
> size will be the system default."
>
Whatever term is actually used, the description should definitely refer to
"The number of hugepages to configure".
On Tue, May 7, 2024 at 3:00 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, May 07, 2024 at 01:43:30PM +0100, Luca Vizzarro wrote:
> > On 07/05/2024 13:05, Bruce Richardson wrote:
> > > Sorry to be late to the reviews here, but since this is a countable value -
> > > as you state in the cover letter- would "number" or "count" not be better
> > > terms. To me, "quantity" is just a synonym of "amount", and can be used for
> > > uncountable values too, e.g. "a quantity of water".
> >
> >
> > Hi Bruce,
> >
> > The change is based on the readability and intuitiveness of the
> > configuration file. In which case "number" could be ambiguous:
> >
> > hugepages_2mb:
> > number: 100
> >
> > And here I could see "count" working:
> >
> > hugepages_2mb:
> > count: 100
> >
We could use number_of: but that doesn't look great. Count looks fine.
> > But since the change is propagated for consistency. "count" would no longer
> > be well fitting in the rest:
> >
> > "description": "The count of hugepages to configure. Hugepage
> > size will be the system default."
> >
> Whatever term is actually used, the description should definitely refer to
> "The number of hugepages to configure".
This makes sense, let's use "number of" in descriptions.
Ideally we'd also use number in code, but it's a bit ambiguous, such as here:
def _configure_huge_pages(self, number: int, size: int,
force_first_numa: bool) -> None:
At a first glance it's not quite clear what "number" is here.
"number_of" would be pretty clear, but so would be "count". But using
count would mean we're using different words with the same meaning in
the same context, which I'd also like to avoid - this is the reason
why I was originally ok with quantity. Now I'm not sure what the best
option is :-)
On Mon, May 13, 2024 at 6:06 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
>
> On Tue, May 7, 2024 at 3:00 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, May 07, 2024 at 01:43:30PM +0100, Luca Vizzarro wrote:
> > > On 07/05/2024 13:05, Bruce Richardson wrote:
> > > > Sorry to be late to the reviews here, but since this is a countable value -
> > > > as you state in the cover letter- would "number" or "count" not be better
> > > > terms. To me, "quantity" is just a synonym of "amount", and can be used for
> > > > uncountable values too, e.g. "a quantity of water".
> > >
> > >
> > > Hi Bruce,
> > >
> > > The change is based on the readability and intuitiveness of the
> > > configuration file. In which case "number" could be ambiguous:
> > >
> > > hugepages_2mb:
> > > number: 100
> > >
> > > And here I could see "count" working:
> > >
> > > hugepages_2mb:
> > > count: 100
> > >
>
> We could use number_of: but that doesn't look great. Count looks fine.
I personally think that number_of is the better option of the two.
Count does work, but to me, it's not as immediately clear as
number_of; syntactically, it makes more sense.
>
> > > But since the change is propagated for consistency. "count" would no longer
> > > be well fitting in the rest:
> > >
> > > "description": "The count of hugepages to configure. Hugepage
> > > size will be the system default."
> > >
> > Whatever term is actually used, the description should definitely refer to
> > "The number of hugepages to configure".
>
> This makes sense, let's use "number of" in descriptions.
I will make the change as requested.
>
> Ideally we'd also use number in code, but it's a bit ambiguous, such as here:
> def _configure_huge_pages(self, number: int, size: int,
> force_first_numa: bool) -> None:
>
> At a first glance it's not quite clear what "number" is here.
> "number_of" would be pretty clear, but so would be "count". But using
> count would mean we're using different words with the same meaning in
> the same context, which I'd also like to avoid - this is the reason
> why I was originally ok with quantity. Now I'm not sure what the best
> option is :-)
Now that you mention it, and given Bruce's comments regarding the use
of quantity, I really like the use of number_of throughout the
framework and even the conf.yaml. Doing so will create consistency in
both the framework's internal documentation (like the 'number of'
suggestion above) and the code, removing the ambiguity that you
mentioned in some of the definitions.
@@ -36,7 +36,7 @@ nodes:
use_first_core: false # tells DPDK to use any physical core
memory_channels: 4 # tells DPDK to use 4 memory channels
hugepages_2mb: # optional; if removed, will use system hugepage configuration
- amount: 256
+ quantity: 256
force_first_numa: false
ports:
# sets up the physical link between "SUT 1"@000:00:08.0 and "TG 1"@0000:00:08.0
@@ -72,7 +72,7 @@ nodes:
peer_node: "SUT 1"
peer_pci: "0000:00:08.1"
hugepages_2mb: # optional; if removed, will use system hugepage configuration
- amount: 256
+ quantity: 256
force_first_numa: false
traffic_generator:
type: SCAPY
@@ -127,11 +127,11 @@ class HugepageConfiguration:
r"""The hugepage configuration of :class:`~framework.testbed_model.node.Node`\s.
Attributes:
- amount: The number of hugepages.
+ quantity: The quantity of hugepages.
force_first_numa: If :data:`True`, the hugepages will be configured on the first NUMA node.
"""
- amount: int
+ quantity: int
force_first_numa: bool
@@ -150,9 +150,9 @@
"type": "object",
"description": "Optional hugepage configuration. If not specified, hugepages won't be configured and DTS will use system configuration.",
"properties": {
- "amount": {
+ "quantity": {
"type": "integer",
- "description": "The amount of hugepages to configure. Hugepage size will be the system default."
+ "description": "The quantity of hugepages to configure. Hugepage size will be the system default."
},
"force_first_numa": {
"type": "boolean",
@@ -161,7 +161,7 @@
},
"additionalProperties": false,
"required": [
- "amount"
+ "quantity"
]
},
"mac_address": {
@@ -37,7 +37,7 @@ class HugepageConfigurationDict(TypedDict):
"""Allowed keys and values."""
#:
- amount: int
+ quantity: int
#:
force_first_numa: bool
@@ -138,7 +138,7 @@ def _supports_numa(self) -> bool:
# there's no reason to do any numa specific configuration)
return len(self._numa_nodes) > 1
- def _configure_huge_pages(self, amount: int, size: int, force_first_numa: bool) -> None:
+ def _configure_huge_pages(self, quantity: int, size: int, force_first_numa: bool) -> None:
self._logger.info("Configuring Hugepages.")
hugepage_config_path = f"/sys/kernel/mm/hugepages/hugepages-{size}kB/nr_hugepages"
if force_first_numa and self._supports_numa():
@@ -149,7 +149,7 @@ def _configure_huge_pages(self, amount: int, size: int, force_first_numa: bool)
f"/hugepages-{size}kB/nr_hugepages"
)
- self.send_command(f"echo {amount} | tee {hugepage_config_path}", privileged=True)
+ self.send_command(f"echo {quantity} | tee {hugepage_config_path}", privileged=True)
def update_ports(self, ports: list[Port]) -> None:
"""Overrides :meth:`~.os_session.OSSession.update_ports`."""
@@ -266,7 +266,7 @@ def _setup_hugepages(self) -> None:
"""
if self.config.hugepages:
self.main_session.setup_hugepages(
- self.config.hugepages.amount,
+ self.config.hugepages.quantity,
self.main_session.hugepage_size,
self.config.hugepages.force_first_numa,
)