Re: [PATCH] hugetlb: Fix hugepages_setup when deal with pernode

[Mike Kravetz <mike.kravetz@oracle.com>]

On 3/24/22 20:15, liupeng (DM) wrote:
> 
> On 2022/3/25 5:57, Mike Kravetz wrote:
>> On 3/24/22 00:40, Peng Liu wrote:
>>> Hugepages can be specified to pernode since "hugetlbfs: extend
>>> the definition of hugepages parameter to support node allocation",
>>> but the following two problems are observed.
>>>
>>> 1) Confusing behavior is observed when both 1G and 2M hugepage
>>> is set after "numa=off".
>>>   cmdline hugepage settings:
>>>    hugepagesz=1G hugepages=0:3,1:3
>>>    hugepagesz=2M hugepages=0:1024,1:1024
>>>   results:
>>>    HugeTLB registered 1.00 GiB page size, pre-allocated 0 pages
>>>    HugeTLB registered 2.00 MiB page size, pre-allocated 1024 pages
>>>
>>> 2) Using invalid option values causes the entire kernel boot option
>>> string to be reported as Unknown.
>>>   Unknown kernel command line parameters "hugepages=0:1024,1:1024"
>> Thank you for debugging and sending the patch!
>>
>> My first thought was "If someone is specifying 'numa=off' as well as
>> numa node specific allocations on the same command line, we should just
>> fail the allocation request".  However, this same situation could exist
>> without the 'numa=off' option as long as an invalid node is included in
>> the list.
> We will "specifying 'numa=off' as well as numa node specific allocations"
> for some debugging and test cases. If the original command line can be
> partly effective, this will be convenient. Yet, we also test "an invalid
> node is included in the list", the behavior is the same with "numa=off".
> 
>> With your patch, the node specific allocations are parsed (and processed)
>> until there is an error.  So, in the example above 3 1G pages and 1024 2M
>> pages are allocated on node 0.  That seems correct.
>>
>> Now suppose the node specific allocations are specified as:
>> hugepagesz=1G hugepages=1:3,0:3
>> hugepagesz=2M hugepages=1:1024,0:1024
> For this case, with/without this patch, huge page will be not allocated
> on any node.
>> Since node 1 is invalid, we experience an error here and do not allocate
>> any pages on node 0.
>>
>> I am wondering if we should just error and ignore the entire string if
>> ANY of the specified nodes are invalid?  Thoughts?
> 
> Thank you for your response.
> 
> This patch only to be consistent between 2M/1G behavior, and repair "return 0"
> as 1d02b444b8d1 ("tracing: Fix return value of __setup handlers").
> With this patch, a node could allocate huge pages until there is an error, and it
> will print the invalid parameter from the first parse error. So, I think this
> is acceptable.

Yes, I agree that the change is needed and the current behavior is
unacceptable.

One remaining question is the change from returning '0' to '1' in the case
of error.  I do understand this is to prevent the invalid parameter string
from being passed to init.  It may not be correct/right, but in every other
case where an invalid parameter in encountered in hugetlb command line
processing we return "0".  Should we perhaps change all these other places
to be consistent?  I honestly do not know what is the appropriate behavior
in these situations.
-- 
Mike Kravetz