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

[Nishanth Aravamudan]

On 26.06.2014 [16:37:05 -0300], Eduardo Habkost wrote:
> On Tue, Jun 24, 2014 at 10:40:38AM -0700, Nishanth Aravamudan 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 */
> 
> I would rename it to num_numa_nodes, so we can easily ensure all code
> using nb_numa_nodes will be converted appropriately.
> 
> > +extern int max_numa_node; /* Highest specified NUMA node ID */
> 
> I would rename it max_numa_nodeid, to make it clear it is the maximum
> ID, not the maximum number of nodes.

Thanks, I'm rebasing onto your series now.

<snip>

> >  int numa_init_func(QemuOpts *opts, void *opaque)
> > @@ -155,7 +162,7 @@ void set_numa_nodes(void)
> >  {
> >      if (nb_numa_nodes > 0) {
> >          uint64_t numa_total;
> > -        int i;
> > +        int i, j = -1;
> 
> Can you please initialize j closer to the loop where it is used?

Yep.

<snip>

> >              /* On Linux, the each node's border has to be 8MB aligned,
> >               * the final node gets the rest.
> >               */
> > -            for (i = 0; i < nb_numa_nodes - 1; i++) {
> > -                numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
> > -                                        ~((1 << 23UL) - 1);
> > -                usedmem += numa_info[i].node_mem;
> > +            for (i = 0; i < max_numa_node - 1; i++) {
> > +                if (numa_info[i].present) {
> > +                    numa_info[i].node_mem = (ram_size / nb_numa_nodes) &
> > +                                            ~((1 << 23UL) - 1);
> > +                    usedmem += numa_info[i].node_mem;
> > +                }
> >              }
> 
> This part is tricky: the following line works only because
> numa_info[max_numa_node-1] is always present:
> 
> >              numa_info[i].node_mem = ram_size - usedmem;
> 
> So, what about adding assert(numa_info[i].present) here?

Yep.

<snip>

> > @@ -203,9 +213,12 @@ void set_numa_nodes(void)
> >           * must cope with this anyway, because there are BIOSes out there 
> > in
> >           * real machines which also use this scheme.
> >           */
> > -        if (i == nb_numa_nodes) {
> > +        if (i == max_numa_node) {
> >              for (i = 0; i < max_cpus; i++) {
> > -                set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
> > +                do {
> > +                    j = (j + 1) % max_numa_node;
> > +                } while (!numa_info[j].present);
> 
> If you change it from "do { } while" to "while { }", you don't need to
> initialize j to -1.

I don't think that's quite as simple as you make it out to be. If you
use a while() loop, we won't always increment j, which means once we've
found a present node, we'll always use that node? j here basically
represents the *last* used nodeid, which we don't want to use again when
we re-enter the for-loop, we want to use the next present nodeid. It
seems like the do {} while() does this fine?  I could use a while() if I
added another increment outside the loop, as follows:

        if (i == max_numa_nodeid) {
            for (i = 0, j = 0; i < max_cpus; i++) {
                while (!numa_info[j].present) {
                    j = (j + 1) % (max_numa_nodeid);
                }
                set_bit(i, numa_info[j].node_cpu);
                j = (j + 1) % (max_numa_nodeid);
            }
        }

If you think that is cleaner, I'll use that version.

Thanks,
Nish