Re: [PATCH 1/2] numa: Set default distance map if needed

[Gavin Shan <gshan@redhat.com>]

Hi Igor,

On 10/12/21 8:40 PM, Igor Mammedov wrote:
> On Wed,  6 Oct 2021 18:22:08 +0800
> Gavin Shan <gshan@redhat.com> wrote:
> 
>> The following option is used to specify the distance map. It's
>> possible the option isn't provided by user. In this case, the
>> distance map isn't populated and exposed to platform. On the
>> other hand, the empty NUMA node, where no memory resides, is
>> allowed on ARM64 virt platform. For these empty NUMA nodes,
>> their corresponding device-tree nodes aren't populated, but
>> their NUMA IDs should be included in the "/distance-map"
>> device-tree node, so that kernel can probe them properly if
>> device-tree is used.
>>
>>    -numa,dist,src=<numa_id>,dst=<numa_id>,val=<distance>
>>
>> So when user doesn't specify distance map, we need to generate
>> the default distance map, where the local and remote distances
>> are 10 and 20 separately. This adds an extra parameter to the
>> exiting complete_init_numa_distance() to generate the default
>> distance map for this case.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
> 
> 
> how about error-ing out if distance map is required but
> not provided by user explicitly and asking user to fix
> command line?
> 
> Reasoning behind this that defaults are hard to maintain
> and will require compat hacks and being raod blocks down
> the road.
> Approach I was taking with generic NUMA code, is deprecating
> defaults and replacing them with sanity checks, which bail
> out on incorrect configuration and ask user to correct command line.
> Hence I dislike approach taken in this patch.
> 
> If you really wish to provide default, push it out of
> generic code into ARM specific one
> (then I won't oppose it that much (I think PPC does
> some magic like this))
> Also behavior seems to be ARM specific so generic
> NUMA code isn't a place for it anyways
> 

Thanks for your comments.

Yep, Lets move the logic into hw/arm/virt in v3 because I think simply
error-ing out will block the existing configuration where the distance
map isn't provided by user. After moving the logic to hw/arm/virt,
this patch is consistent with PATCH[02/02] and the specific platform
is affected only.


>> ---
>>   hw/core/numa.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/numa.c b/hw/core/numa.c
>> index 510d096a88..fdb3a4aeca 100644
>> --- a/hw/core/numa.c
>> +++ b/hw/core/numa.c
>> @@ -594,7 +594,7 @@ static void validate_numa_distance(MachineState *ms)
>>       }
>>   }
>>   
>> -static void complete_init_numa_distance(MachineState *ms)
>> +static void complete_init_numa_distance(MachineState *ms, bool is_default)
>>   {
>>       int src, dst;
>>       NodeInfo *numa_info = ms->numa_state->nodes;
>> @@ -609,6 +609,8 @@ static void complete_init_numa_distance(MachineState *ms)
>>               if (numa_info[src].distance[dst] == 0) {
>>                   if (src == dst) {
>>                       numa_info[src].distance[dst] = NUMA_DISTANCE_MIN;
>> +                } else if (is_default) {
>> +                    numa_info[src].distance[dst] = NUMA_DISTANCE_DEFAULT;
>>                   } else {
>>                       numa_info[src].distance[dst] = numa_info[dst].distance[src];
>>                   }
>> @@ -716,13 +718,20 @@ void numa_complete_configuration(MachineState *ms)
>>            * A->B != distance B->A, then that means the distance table is
>>            * asymmetric. In this case, the distances for both directions
>>            * of all node pairs are required.
>> +         *
>> +         * The default node pair distances, which are 10 and 20 for the
>> +         * local and remote nodes separatly, are provided if user doesn't
>> +         * specify any node pair distances.
>>            */
>>           if (ms->numa_state->have_numa_distance) {
>>               /* Validate enough NUMA distance information was provided. */
>>               validate_numa_distance(ms);
>>   
>>               /* Validation succeeded, now fill in any missing distances. */
>> -            complete_init_numa_distance(ms);
>> +            complete_init_numa_distance(ms, false);
>> +        } else {
>> +            complete_init_numa_distance(ms, true);
>> +            ms->numa_state->have_numa_distance = true;
>>           }
>>       }
>>   }
> 

Thanks,
Gavin