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
