Re: [Qemu-devel] [RFC PATCH v3] numa: enable sparse node numbering

[Igor Mammedov]

On Wed, 25 Jun 2014 10:04:18 -0700
Nishanth Aravamudan <address@hidden> wrote:

> On 25.06.2014 [13:52:56 -0300], Eduardo Habkost wrote:
> > On Wed, Jun 25, 2014 at 09:13:59AM -0700, Nishanth Aravamudan wrote:
> > > On 25.06.2014 [13:21:34 +0200], Igor Mammedov wrote:
> > > > On Tue, 24 Jun 2014 10:40:38 -0700
> > > > Nishanth Aravamudan <address@hidden> wrote:
> > > <snip>
> > > > > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> > > > > index 277230d..b90bf66 100644
> > > > > --- a/include/sysemu/sysemu.h
> > > > > +++ b/include/sysemu/sysemu.h
> > > > > @@ -145,11 +145,13 @@ extern int mem_prealloc;
> > > > >   */
> > > > >  #define MAX_CPUMASK_BITS 255
> > > > >  
> > > > > -extern int nb_numa_nodes;
> > > > > +extern int nb_numa_nodes; /* Number of NUMA nodes */
> > > > > +extern int max_numa_node; /* Highest specified NUMA node ID */
> > > > >  typedef struct node_info {
> > > > >      uint64_t node_mem;
> > > > >      DECLARE_BITMAP(node_cpu, MAX_CPUMASK_BITS);
> > > > >      struct HostMemoryBackend *node_memdev;
> > > > > +    bool present;
> > > > How about dropping 'present' and replacing array with a list
> > > > of only present nodes?
> > > 
> > > If that would be preferred, I can move to that. I assume a simple
> > > linked-list is fine. Does qemu provide any infrastructure for defining
> > > lists? I'll look through the source but any pointers would be helpful.
> > > 
> > > Generally speaking, sparse NUMA nodes aren't that common and when they
> > > exist, the gaps aren't large. But it does seem to make sense if we have
> > > sparse IDs at all, we might as well move to a list.
> > > 
> > > In any case, moving to the list means we'd have a nodeid as part of the
> > > structure instead.
> > > 
> > > > That way it will be one more step closer to converting numa
> > > > infrastructure to a set of QOM objects.
> > > 
> > > Sounds like a good idea to me. I'll respin the patch soon.
> > 
> > Having a list makes sense, the only difference is that keeping a sparse
> > array sorted is much easier than making a sorted list (because the ACPI
> > tables are nodeid-ordered). That's why I suggested keeping the array
> > initially.
> 
> And for non-ACPI platforms, it does feel like keeping the list sorted is
> ideal, as it simplifies various loops, etc.
> 
> > Adding a "present" field to the array is a trivial and easy-to-review
> > change. Changing NodeInfo to use linked lists is a more complex change
> > that I wouldn't want to include after soft freeze.
> > 
> > In other words:
> >  * Having a list is better than a sparse array; but:
> >  * Having a small sparse array with the "present" field is better
> >    than broken sparse nodeid support (IMO).
> 
> Perhaps as a compromise I can work on the list conversion as a follow-on
> patch?
Sure, it was just a suggestion that might make our lives easier in the future. 

> 
> Thanks,
> Nish
>