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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

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.

> > +    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. Per my info, there is only one boot cpu
no matter how many sockets there are.

> > +        return;
> > +    else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT )
> 
> Pointless "else".
> 
Thanks, will remove it.

> > +    {
> > +        __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability);
> 
> setup_clear_cpu_cap() if you use boot_cpu_has() above.
> 
> > +        return;
> > +    }
> > +
> > +    socket = cpu_to_socket(cpu);
> > +    info = socket_info + socket;
> > +    if ( info->feat_mask )
> > +        return;
> > +
> > +    INIT_LIST_HEAD(&info->feat_list);
> > +    spin_lock_init(&info->ref_lock);
> > +
> > +    cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, &regs);
> > +    if ( regs.b & PSR_RESOURCE_TYPE_L3 )
> > +    {
> > +        cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, &regs);
> > +
> > +        feat = feat_l3_cat;
> > +        /* psr_cpu_prepare will allocate it on subsequent CPU onlining. */
> > +        feat_l3_cat = NULL;
> 
> I don't think the comment is very useful: You've consumed the object,
> so you simply should not leave a dangling pointer (or else you'd risk
> multiple use).
> 
Will remove it.

> >  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'. If we move
codes of 'cpu_init_work' into 'psr_cpu_init', the codes look messy. That is
the reason we define 'cpu_init_work'. Do you think if it is acceptable to you?

> Jan

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