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

[Michael S. Tsirkin]

On Wed, Jun 25, 2014 at 01:52:56PM -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.
> 
> 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).

I agree here. This patchset is still RFC but if that's
the only issue I might apply it.

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

Whether this would be a win for the community depends on the specifics
of each case. Contributors are often inclined not to look beyond their
immediate needs and their specific small project.  Who gets to clean it
up then?

> -- 
> Eduardo