Re: [Qemu-devel] [PATCH v2 4/5] numa: fallback to default NUMA node instead of node 0

[Igor Mammedov]

On Fri, 26 May 2017 11:58:14 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, May 23, 2017 at 05:44:03PM +0200, Igor Mammedov wrote:
> > On Tue, 23 May 2017 11:48:54 -0300
> > Eduardo Habkost <address@hidden> wrote:
> >   
> > > On Tue, May 23, 2017 at 04:38:49PM +0200, Igor Mammedov wrote:  
> > > > Do the same as we did in commit
> > > >   (57924bcd8 numa: introduce machine callback for VCPU to node mapping)
> > > > but only for incomplete mapping usecase, falling back to board
> > > > provided default cpu to node mapping if user hasn't provided
> > > > mapping for CPU explicitly.
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>  
> > > 
> > > This breaks migration compatibility, doesn't it? I understand
> > > this use case is not supported, but if are going to break stuff
> > > for people using incomplete mappings (by rejecting the
> > > configuration on a future release), I would prefer to break it
> > > only once.  
> > it's firmware change and we generally don't care nor maintain firmware 
> > compat stuff
> > the same as with above mentioned 
> >  (57924bcd8 numa: introduce machine callback for VCPU to node mapping)  
> 
> 
> We normally keep resulting NUMA topology compatible, even if NUMA
> information is implemented only by firmware.  See how we set
> MachineClass::numa_auto_assign_ram and
> MachineClass::numa_mem_align_shift in older machine-types, for
> example.
it's relative new and pure firmaware change wrt arm/i386,
but it might influence ABI for spapr, my guess is that's why
it was compat flags were introduced
(otherwise they shouldn't have been added).
So I wouldn't consider this precedent as policy change
wrt firmware changes.

 
> Sometimes we didn't keep compatibility, though (e.g. on commit
> fb43b73b92 "pc: fix default VCPU to NUMA node mapping").
and more
(27111931 acpi: provide _PXM method for CPU devices if QEMU is started numa 
enabled)
and this sometimes is more consistent with other firmware
changes that are not versioned and the general approach
(I don't see why numa is any more special that any other
subsystem).

> I wouldn't disagree with this change if we planned to support
> incomplete mappings forever.  But if we are going to remove
> support for incomplete mappings soon, I would prefer to break
> user expectations only once.
My take on why we can't remove incomplete mappings without
obsoleting them first is not because of firmware changes but
that it breaks wrong CLI (i.e. scripts with incomplete mapping
will fail to start guest). So it's not the same vs firmware
behavior change.

I'm agreeing to drop this patch just because I don't care much
about this particular fixup as it's small isolated part in one place
and doesn't affect the rest of the code.


> > >   
> > > > ---
> > > >  hw/core/machine.c | 3 ---
> > > >  1 file changed, 3 deletions(-)
> > > > 
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index 964b75d..b8df15f 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -730,9 +730,6 @@ static void machine_numa_finish_init(MachineState 
> > > > *machine)
> > > >                  g_string_append_printf(s, "%sCPU %d [%s]",
> > > >                                         s->len ? ", " : "", i, cpu_str);
> > > >                  g_free(cpu_str);
> > > > -
> > > > -                /* non mapped cpus used to fallback to node 0 */
> > > > -                props.node_id = 0;
> > > >              }
> > > >  
> > > >              props.has_node_id = true;
> > > > -- 
> > > > 2.7.4
> > > >   
> > >   
> > 
> >   
>