On Tue, Oct 12, 2021 at 11:40:16AM +0200, Igor Mammedov wrote: > On Wed, 6 Oct 2021 18:22:08 +0800 > Gavin Shan <gs...@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 <gs...@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
The distance-map DT node and the default 10/20 distance-map values aren't arch-specific. RISCV is using it too. I'm on the fence with this. I see erroring-out to require users to provide explicit command lines as a good thing, but I also see it as potentially an unnecessary burden for those that want the default map anyway. The optional nature of the distance-map node and the specification of the default map is here [1] [1] Linux source: Documentation/devicetree/bindings/numa.txt So, my r-b stands for this patch, but I also wouldn't complain about respinning it to error out instead. I would complain about moving the logic to Arm specific code, though, since RISCV would then need to duplicate it. Thanks, drew > > > --- > > 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; > > } > > } > > } >