qemu-devel
[Top][All Lists]
Advanced

[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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]