I fixed the docstring under setup_hugepages in os_session, and I also
made a quick fix to the dts.rst documentation. For the dts.rst
documentation, I think the following changes make more sense, based on
the concerns outlined:

(here is a snip of the documentation with the change I made)
"as doing so prevents accidental/over
allocation of with hugepage sizes not recommended during runtime due to
contiguous memory space requirements."

With regard to the wording used for the total number of hugepages, I
could change the wording to either "quantity" or "number;" I think
quantity makes more sense and is less ambiguous, but I'm curious what
you think. With reference to your comments about putting this in a
different patch set, I think a good argument could be made to put this
kind of a change in out currently existing patch, but I understand the
argument at both ends. Personally, I am in favor of adding this fix to
the current patch since we're renaming key/value pairs in the schema
and yaml already.

As far as the property is concerned, when Jeremy and I discussed how
to best implement this fix, he suggested that a property might make
more sense here because of the potential changes that we might make to
the default size in the future (whether that be by OS, arch or
otherwise). Ultimately, we settled on inserting a property that
returns 2048 for now with the understanding that, in the future,
developers can add logic to the property as needed. Initially, I had
the hugepage size configured in the manner you described, so the
property implementation is not something I'm adamant on. I can make
the suggested change you gave above, or alternatively, if my provided
reasoning makes sense, I can insert a comment exclaiming the existence
of the property.

Reply via email to