[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] numa: Set default distance map if needed
From: |
Andrew Jones |
Subject: |
Re: [PATCH 1/2] numa: Set default distance map if needed |
Date: |
Tue, 12 Oct 2021 12:37:54 +0200 |
On Tue, Oct 12, 2021 at 11:40:16AM +0200, 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
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;
> > }
> > }
> > }
>
- Re: [PATCH 1/2] numa: Set default distance map if needed, (continued)
Re: [PATCH 1/2] numa: Set default distance map if needed, Igor Mammedov, 2021/10/12
- Re: [PATCH 1/2] numa: Set default distance map if needed, Gavin Shan, 2021/10/12
- Re: [PATCH 1/2] numa: Set default distance map if needed, Igor Mammedov, 2021/10/12
- Re: [PATCH 1/2] numa: Set default distance map if needed, Andrew Jones, 2021/10/12
- Re: [PATCH 1/2] numa: Set default distance map if needed, Igor Mammedov, 2021/10/12
- Re: [PATCH 1/2] numa: Set default distance map if needed, Andrew Jones, 2021/10/12
- Re: [PATCH 1/2] numa: Set default distance map if needed, Gavin Shan, 2021/10/12
Re: [PATCH 1/2] numa: Set default distance map if needed,
Andrew Jones <=
Re: [PATCH 1/2] numa: Set default distance map if needed, Igor Mammedov, 2021/10/12
Re: [PATCH 1/2] numa: Set default distance map if needed, Andrew Jones, 2021/10/12
Re: [PATCH 1/2] numa: Set default distance map if needed, Igor Mammedov, 2021/10/12
Re: [PATCH 1/2] numa: Set default distance map if needed, Gavin Shan, 2021/10/12
Re: [PATCH 1/2] numa: Set default distance map if needed, Igor Mammedov, 2021/10/13
Re: [PATCH 1/2] numa: Set default distance map if needed, Andrew Jones, 2021/10/13