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

[Eduardo Habkost]

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.

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).

<rant>
Improving the code is always welcome (and adding a list would be an
improvement), but we don't need to implement all refactoring changes we
can dream of, before including a new feature. I like to evaluate how the
code looks like compared to what we have today, not with what we want it
to look like in one year.

Could we (the QEMU community) please learn to sometimes accept simple
features or fixes (which don't make the code worse) without requiring
the whole code to be rewritten first?
</rant>

-- 
Eduardo