[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/3] numa: move default mapping init to machine
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH 2/3] numa: move default mapping init to machine |
Date: |
Mon, 22 May 2017 10:09:00 -0300 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, May 22, 2017 at 09:04:03AM +0200, Igor Mammedov wrote:
> On Thu, 18 May 2017 15:50:58 -0300
> Eduardo Habkost <address@hidden> wrote:
> > On Thu, May 18, 2017 at 10:09:30AM +0200, Igor Mammedov wrote:
[...]
> >
> > > + default_mapping = !i; /* i == 0 : no explicit mapping provided by
> > > user */
> > > +
> > > for (i = 0; i < possible_cpus->len; i++) {
> > > const CPUArchId *cpu_slot = &possible_cpus->cpus[i];
> > >
> > > - /* at this point numa mappings are initilized by CLI options
> > > - * or with default mappings so it's sufficient to list
> > > - * all not yet mapped CPUs here */
> > > - /* TODO: make it hard error in future */
> >
> > Did we change our mind about making it a hard error in the
> > future?
> nope, error message at the end of function says that partial mapping
> is obsoleted and will be removed. I've thought that's was sufficient
> reminder for us of what needs to be removed.
Yes, it is sufficient. I just didn't notice the message. :)
> I'll move TODO to:
>
> + } else {
> + /* record slots with not set mapping */
> ++ /* TODO: make it hard error in future */
> + char *cpu_str = cpu_slot_to_string(cpu_slot);
> + g_string_append_printf(s, "%sCPU %d [%s]",
I don't mind removing the TODO comment, so it's up to you.
>
> >
> > > if (!cpu_slot->props.has_node_id) {
> > > - char *cpu_str = cpu_slot_to_string(cpu_slot);
> > > - g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " :
> > > "", i,
> > > - cpu_str);
> > > - g_free(cpu_str);
> > > + if (default_mapping) {
> > > + /* fetch default mapping from board and enable it */
> > > + CpuInstanceProperties props = cpu_slot->props;
> > > + props.has_node_id = true;
> > > + machine_set_cpu_numa_node(machine, &props,
> > > &error_fatal);
> >
> > Is a machine_set_cpu_numa_node() call really necessary here, if
> > we are already looking at cpu_slot->props directly?
> yes, it's necessary as cpu_slot comes from:
> const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
> callback, which initializes machine::possible_cpus if necessary.
>
> So cpu_slot is readonly and I'd prefer to keep mc->possible_cpu_arch_ids()
> returning 'const' as it's used by external callers and they shouldn't
> mess with possible_cpus content by accident.
>
> usage of machine_set_cpu_numa_node() adds +2 more lines and it's
> proper/validating setter for node_id and will catch
> mistakes if we make them in the code.
> So I wouldn't use shortcuts here to save 2 lines.
Oh, I didn't notice cpu_slot was const. That's OK to me.
>
>
> > > + } else {
> > > + /* record slots with not set mapping */
> > > + char *cpu_str = cpu_slot_to_string(cpu_slot);
> > > + g_string_append_printf(s, "%sCPU %d [%s]",
> > > + s->len ? ", " : "", i, cpu_str);
> > > + g_free(cpu_str);
> > > + }
> > > }
> >
> > What about doing this instead:
> >
> > if (default_mapping) {
> > /*
> > * Default mapping was already set by board at
> > * cpu_slot->props.node_id, just enable it
> > */
> > cpu_slot->props.has_node_id = true;
> > } else if (!cpu_slot->props.has_node_id) {
> > char *cpu_str = cpu_slot_to_string(cpu_slot);
> > g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "",
> > i,
> > cpu_str);
> > g_free(cpu_str);
> > }
--
Eduardo
Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr, David Gibson, 2017/05/18
[Qemu-devel] [PATCH 3/3] numa: silence incomplete mapping warning under qtest, Igor Mammedov, 2017/05/18