Re: [PATCH 1/1] numa: check mem or memdev in numa configuration

[Li Zhang <lizhang@suse.de>]

On 2/17/22 11:25 AM, Igor Mammedov wrote:
> On Thu, 17 Feb 2022 10:38:32 +0100
> Li Zhang <lizhang@suse.de> wrote:
> 
>> On 2/17/22 10:10 AM, Igor Mammedov wrote:
>>> On Wed, 16 Feb 2022 17:36:13 +0100
>>> Li Zhang <lizhang@suse.de> wrote:
>>>    
>>>> If there is no mem or memdev in numa configuration, it always
>>>> reports the error as the following:
>>>>
>>>> total memory for NUMA nodes (0x0) should equal RAM size (0x100000000)
>>>>
>>>> This error is confusing and the reason is that total memory of numa nodes
>>>> is always 0 if there is no mem or memdev in numa configuration.
>>>> So it's better to check mem or memdev in numa configuration.
>>>>
>>>> Signed-off-by: Li Zhang <lizhang@suse.de>
>>>> ---
>>>>    hw/core/numa.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/hw/core/numa.c b/hw/core/numa.c
>>>> index 1aa05dcf42..11cbec51eb 100644
>>>> --- a/hw/core/numa.c
>>>> +++ b/hw/core/numa.c
>>>> @@ -132,6 +132,11 @@ static void parse_numa_node(MachineState *ms, NumaNodeOptions *node,
>>>>    
>>>>        have_memdevs = have_memdevs ? : node->has_memdev;
>>>>        have_mem = have_mem ? : node->has_mem;
>>>> +    if (!node->has_memdev && !node->has_mem) {
>>>> +        error_setg(errp, "numa configuration should use mem= or memdev= ");
>>>> +        return;
>>>> +    }
>>>
>>> Wouldn't this breaks memory less numa nodes?
>>
>> Yes, you are right. It will break it if there more numa nodes
>> than memory, and the numa nodes have no memory backends specified.
>>
>> Is it allowed for users to specify more numa nodes than memory?
> yep, I think we support it at least for one of the targets
> (but I don't remember which one(s))
> 

Is it okay if I put a warning here, instead of an error and return?
It won't break the special case. I wonder if it is annoying to get
the warning.

Thanks
Li

>>
>>>
>>> I'd rather add/rephrase to original error message that memory
>>> should be specified explicitly for desired numa nodes.
>>> And I'd not mention 'mem=' since
>>>     docs/about/removed-features.rst:``-numa node,mem=...`` (removed in 5.1)
>>
>> Thanks for your suggestions, I will rephrase it.
>>
>>>    
>>>> +
>>>>        if ((node->has_mem && have_memdevs) || (node->has_memdev && have_mem)) {
>>>>            error_setg(errp, "numa configuration should use either mem= or memdev=,"
>>>>                       "mixing both is not allowed");
>>>    
>>
>