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

[Gavin Shan <gshan@redhat.com>]

Hi Drew and Igor,

On 10/13/21 12:05 AM, Andrew Jones wrote:
> On Tue, Oct 12, 2021 at 02:34:30PM +0200, Igor Mammedov wrote:
>> On Tue, 12 Oct 2021 13:48:02 +0200
>>> On Tue, Oct 12, 2021 at 09:31:55PM +1100, Gavin Shan wrote:
>>>> 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.
>>>
>>> Please don't move anything NUMA DT generic to hw/arm/virt. If the spec
>>> isn't arch-specific, then the modeling shouldn't be either.
>>
>>
>>> If you want to error-out for all configs missing the distance map, then
>>> you'll need compat code.
>>
>>> If you only want to error-out for configs that
>>> have empty NUMA nodes and are missing a distance map, then you don't
>>> need compat code, because those configs never worked before anyway.
>>
>> I think memory-less configs without distance map worked for x86 just fine.
> 
> Ah, yes, we should make the condition for erroring-out be
> 
>   have-memoryless-nodes && !have-distance-map && generate-DT
> 
> ACPI only architectures, x86, don't need to care about this.
> 

Sure, I will change the code accordingly in v3. Thanks for discussing
it through with Igor :)

>>
>> After looking at this thread all over again it seems to me that using
>> distance map as a source of numa ids is a mistake.
> 
> You'll have to discuss that with Rob Herring, as that was his proposal.
> He'll expect a counterproposal though, which we don't have...
> 

However, Getting the NUMA node IDs from PCI host bridge and CPUs aren't
working out. I will explain in another thread.

Thanks,
Gavin