Re: [Qemu-devel] [PATCH v2 1/2] numa: introduce numa_auto_assign_ram() function in MachineClass

[Eduardo Habkost]

On Thu, Apr 27, 2017 at 05:09:26PM +0200, Laurent Vivier wrote:
> On 27/04/2017 16:09, Eduardo Habkost wrote:
> > On Thu, Apr 27, 2017 at 12:12:58PM +0200, Laurent Vivier wrote:
> >> We need to change the way we distribute the memory across
> >> the nodes. To keep compatibility between machine type version
> >> introduce a  machine type dependent function.
> >>
> >> Signed-off-by: Laurent Vivier <address@hidden>
> > [...]
> >> +static void numa_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >> +                                 int nb_nodes, ram_addr_t size)
> >> +{
> >> +    int i;
> >> +    uint64_t usedmem = 0;
> >> +
> >> +    if (mc->numa_auto_assign_ram) {
> >> +        uint64_t *mem = g_new(uint64_t, nb_nodes);
> >> +
> >> +        mc->numa_auto_assign_ram(mem, nb_nodes, size);
> >> +
> >> +        for (i = 0; i < nb_nodes; i++) {
> >> +            nodes[i].node_mem = mem[i];
> >> +        }
> >> +
> >> +        g_free(mem);
> >> +
> >> +        return;
> >> +    }
> >> +
> >> +    /* Align each node according to the alignment
> >> +     * requirements of the machine class
> >> +     */
> >> +
> >> +    for (i = 0; i < nb_nodes - 1; i++) {
> >> +        nodes[i].node_mem = (size / nb_nodes) &
> >> +                            ~((1 << mc->numa_mem_align_shift) - 1);
> >> +        usedmem += nodes[i].node_mem;
> >> +    }
> >> +    nodes[i].node_mem = size - usedmem;
> >> +}
> > 
> > I would prefer to make your new algorithm the default, and move
> > this code to a legacy_auto_assign_ram() function set by the 2.9
> > machine-types.
> 
> I think it's easier to do as I've done because otherwise, we need:
> 
> - to add the numa_mem_align_shift to the parameters list of the
>   numa_auto_assign_ram() function.

You can take MachineClass or MachineState as paramter.

> 
> - set the function by default to numa_auto_assign_ram in
>   hw/core/machine.c:machine_class_init()

Yep.

> 
> - set the pointer to NULL in 2.10 pseries machine type,

Won't pseries-2.10 have the same behavior as all other machines
except pc/pseries <= 2.9? pseries-2.10 and pc-2.10 would just
reuse the default value set by machine_class_init().

> 
> - export the function to re-set the legacy function in the 2.9 pseries
>   machine type definition.

Well, this is the part where I agree it's too much hassle.  :)

> 
> So, we need to add one argument to the function, export the function to
> use it from machine.c and at least spapr.c, to set the function in
> machine_class_init() and spapr_machine_2_9_class_options() (as we clear
> it in 2.10 function).
> 
> I can do that, but is this what you want?

I don't have a strong opinion. If you believe your way is
simpler, we can keep it.

-- 
Eduardo