On Tue, 12 Oct 2021 13:48:02 +0200
Andrew Jones <drjo...@redhat.com> wrote:
> On Tue, Oct 12, 2021 at 09:31:55PM +1100, Gavin Shan wrote:
> > Hi Igor,
> >
> > On 10/12/21 8:40 PM, 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
> > >
> >
> > 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.
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.
>
> 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;
> > > > }
> > > > }
> > > > }
> > >
> >
> > Thanks,
> > Gavin
> >
>
