Re: [Xen-devel] [PATCH v8 04/24] x86: refactor psr: implement CPU init and free flow.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.17 at 02:32, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-03-08 07:56:54, Jan Beulich wrote:
>> >>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > -static int psr_cpu_prepare(unsigned int cpu)
>> > +static void cpu_init_work(void)
>> > +{
>> > +    struct psr_socket_info *info;
>> > +    unsigned int socket;
>> > +    unsigned int cpu = smp_processor_id();
>> > +    struct feat_node *feat;
>> > +    struct cpuid_leaf regs = { .a = 0, .b = 0, .c = 0, .d = 0 };
>> 
>> I don't see you needing an initializer here at all, but if you really
>> want one for some reason, the same effect can be had with just
>> {}.
>> 
> Konrad suggested me to initialize it like this in v5 patch review comments.
> I think he has experienced some strange issues when he forgot to set _all_
> the entries a structure allocated on the stack.

I can see there being cases where this is desirable; I don't think
this is one of them. (See also a related comment I had made on
a later patch.)

>> > +    if ( !cpu_has(&current_cpu_data, X86_FEATURE_PQE) )
>> 
>> Do you really mean to not universally check the global (boot CPU)
>> flag? I.e. possibly differing behavior on different CPUs?
>> 
> Yes, different sockets may have different configurations. E.g. sockt 0 has
> PQE support but socket 1 does not.

For all other CPU features we assume symmetry. Why would we
not do so here? And how would things even work in that case,
when a vCPU gets moved (by the scheduler) from a more capable
to a less capable pCPU?

>> >  static void psr_cpu_init(void)
>> >  {
>> > +    if ( socket_info )
>> > +        cpu_init_work();
>> > +
>> >      psr_assoc_init();
>> >  }
>> >  
>> >  static void psr_cpu_fini(unsigned int cpu)
>> >  {
>> > +    if ( socket_info )
>> > +        cpu_fini_work(cpu);
>> >      return;
>> >  }
>> 
>> Is it really useful to use another layer of helper functions here?
>> 
> The reason we define 'cpu_fini_work' is to match 'cpu_init_work'.

And the question was for both of them.

> If we move
> codes of 'cpu_init_work' into 'psr_cpu_init', the codes look messy.

I don't think that's the case; I could see this as a valid argument if
the calling functions above were already quite complex.

> That is
> the reason we define 'cpu_init_work'. Do you think if it is acceptable to 
> you?

Well, I won't NAK the patch if you keep them, but I'd prefer the
functions to be folded into their callers.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel