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

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

On 17-01-10 04:45:05, Jan Beulich wrote:
> >>> On 14.12.16 at 05:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > @@ -141,11 +144,79 @@ struct psr_assoc {
> >  
> >  struct psr_cmt *__read_mostly psr_cmt;
> >  
> > +static struct psr_socket_info *__read_mostly socket_info;
> > +
> >  static unsigned int opt_psr;
> >  static unsigned int __initdata opt_rmid_max = 255;
> > +static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_CNT;
> >  static uint64_t rmid_mask;
> >  static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
> >  
> > +/* Declare feature list entry. */
> > +static struct feat_node *feat_l3_cat;
> 
> Hmm, if you indeed (again) need such a helper object, then please
> make the comment actually say so. As it is, the comment is mostly
> meaningless.
> 
Thanks! Will add more comments to explain it.

> > +/* Common functions. */
> > +static void free_feature(struct psr_socket_info *info)
> > +{
> > +    struct feat_node *feat_tmp;
> > +
> > +    if ( !info )
> > +        return;
> > +
> > +    list_for_each_entry(feat_tmp, &info->feat_list, list)
> > +    {
> > +        clear_bit(feat_tmp->feature, &info->feat_mask);
> > +        list_del(&feat_tmp->list);
> > +        xfree(feat_tmp);
> > +    }
> 
> This requires list_for_each_entry_safe() to be used, to avoid a
> use-after-free issue (or alternatively a while(!list_empty()) loop).
> 
Thanks for the suggestion!

> > +    /* Free feature which are not added into feat_list. */
> > +    if ( feat_l3_cat )
> > +    {
> > +        xfree(feat_l3_cat);
> > +        feat_l3_cat = NULL;
> > +    }
> 
> Why don't you leave this around, avoiding the need for an
> allocation the next time a CPU comes online? Also note that xfree()
> deals fine with a NULL input, so conditionals like this are pointless.
> 
Thanks! Will keep them.

> > +/* L3 CAT callback functions implementation. */
> > +static void l3_cat_init_feature(unsigned int eax, unsigned int ebx,
> > +                                unsigned int ecx, unsigned int edx,
> 
> This is rather unfortunate naming: How does the reader of this code
> know what these values represent, without having to first go look in
> the caller?
> 
Do you mean the 'eax'-'edx'? How about 'eax_register'?

> > +                                struct feat_node *feat,
> > +                                struct psr_socket_info *info)
> > +{
> > +    struct psr_cat_hw_info l3_cat;
> > +    unsigned int socket;
> > +
> > +    /* No valid value so do not enable feature. */
> > +    if ( !eax || !edx )
> > +        return;
> > +
> > +    l3_cat.cbm_len = (eax & CAT_CBM_LEN_MASK) + 1;
> > +    l3_cat.cos_max = min(opt_cos_max, edx & CAT_COS_MAX_MASK);
> > +
> > +    /* cos=0 is reserved as default cbm(all ones). */
> > +    feat->cos_reg_val[0] = (1ull << l3_cat.cbm_len) - 1;
> 
> Considering how cbm_len gets calculated a few lines up, I can't see
> how this can end up being all ones (as the comment says). At most
> this can be 0xffffffff (as a 64-bit value) afaics.
> 
Sorry for the confusion. All one means all bits within cbm_len are 1. E.g.
the cbm_len is 11. Then, value of cos_reg_val[0] is '(1 << 11) - 1', equals
0x7ff.

Will correct the comment.

> > +    feat->feature = PSR_SOCKET_L3_CAT;
> > +    __set_bit(PSR_SOCKET_L3_CAT, &info->feat_mask);
> > +
> > +    feat->info.l3_cat_info = l3_cat;
> > +
> > +    info->nr_feat++;
> > +
> > +    /* Add this feature into list. */
> > +    list_add_tail(&feat->list, &info->feat_list);
> > +
> > +    socket = cpu_to_socket(smp_processor_id());
> > +    printk(XENLOG_INFO "L3 CAT: enabled on socket %u, cos_max:%u, 
> > cbm_len:%u\n",
> > +           socket, feat->info.l3_cat_info.cos_max,
> > +           feat->info.l3_cat_info.cbm_len);
> 
> I don't think we want such printed for every socket, at least not by
> default. Please, if you want to keep it, make it dependent upon e.g.
> opt_cpu_info.
> 
Thanks! Will limit the print by opt_cpu_info.

> > +}
> > +
> > +struct feat_ops l3_cat_ops = {
> 
> static const
> 
Ok, thanks!

> > @@ -340,18 +414,113 @@ void psr_domain_free(struct domain *d)
> >      psr_free_rmid(d);
> >  }
> >  
> > -static int psr_cpu_prepare(unsigned int cpu)
> > +static int cpu_prepare_work(unsigned int cpu)
> >  {
> > +    if ( !socket_info )
> > +        return 0;
> > +
> > +    /* Malloc memory for the global feature head here. */
> > +    if ( feat_l3_cat == NULL &&
> > +         (feat_l3_cat = xzalloc(struct feat_node)) == NULL )
> > +        return -ENOMEM;
> > +
> >      return 0;
> >  }
> >  
> > +static void cpu_init_work(void)
> > +{
> > +    unsigned int eax, ebx, ecx, edx;
> > +    struct psr_socket_info *info;
> > +    unsigned int socket;
> > +    unsigned int cpu = smp_processor_id();
> > +    const struct cpuinfo_x86 *c = cpu_data + cpu;
> 
> Please use current_cpu_data instead of open coding it.
> 
Thanks for the suggestion!

> > +    struct feat_node *feat_tmp;
> 
> Looking at the uses, I don't think this is temporary in any way - why
> not just "feat"?
> 
No problem, thanks!

> > +    if ( !cpu_has(c, X86_FEATURE_PQE) || c->cpuid_level < 
> > PSR_CPUID_LEVEL_CAT )
> > +        return;
> 
> Instead of such a double check, please consider clearing the PQE
> feature bit when the maximum CPUID level is too low (which
> shouldn't happen anyway).
> 
Is this the responsibility of psr.c? X86_FEATURE_PQE bit is set by HW. Even the
bit is set but CPUID level is low, I think SW would be better to keep it but
not clear it. Because it indicates the HW capability. How do you think? Thanks!

> > +    socket = cpu_to_socket(cpu);
> > +    info = socket_info + socket;
> > +    if ( info->feat_mask )
> > +        return;
> > +
> > +    spin_lock_init(&info->ref_lock);
> > +
> > +    cpuid_count(PSR_CPUID_LEVEL_CAT, 0, &eax, &ebx, &ecx, &edx);
> > +    if ( ebx & PSR_RESOURCE_TYPE_L3 )
> > +    {
> > +        cpuid_count(PSR_CPUID_LEVEL_CAT, 1, &eax, &ebx, &ecx, &edx);
> > +
> > +        feat_tmp = feat_l3_cat;
> > +        feat_l3_cat = NULL;
> > +        feat_tmp->ops = l3_cat_ops;
> > +
> > +        feat_tmp->ops.init_feature(eax, ebx, ecx, edx, feat_tmp, info);
> 
> What's the point of the indirect call here, when you know the
> function is l3_cat_init_feature()?
> 
Hmm, just want to keep the callback function calling style.

> > +static void cpu_fini_work(unsigned int cpu)
> > +{
> > +    unsigned int socket = cpu_to_socket(cpu);
> > +
> > +    if ( !socket_cpumask[socket] || cpumask_empty(socket_cpumask[socket]) )
> > +    {
> > +        struct psr_socket_info *info = socket_info + socket;
> > +
> > +        free_feature(info);
> 
> Pointless local variable "info", unless later patches add further uses.
> 
Ok, will remove this variable. Thanks!

> > +static void __init init_psr(void)
> > +{
> > +    unsigned int i;
> > +
> > +    if ( opt_cos_max < 1 )
> > +    {
> > +        printk(XENLOG_INFO "CAT: disabled, cos_max is too small\n");
> > +        return;
> > +    }
> > +
> > +    socket_info = xzalloc_array(struct psr_socket_info, nr_sockets);
> > +
> > +    if ( !socket_info )
> > +    {
> > +        printk(XENLOG_INFO "Fail to alloc socket_info!\n");
> > +        return;
> > +    }
> > +
> > +    for ( i = 0; i < nr_sockets; i++ )
> > +        INIT_LIST_HEAD(&socket_info[i].feat_list);
> 
> Please decide for one central place where to do such initialization:
> This and spin_lock_init() really should live together (and I think
> better there, not here).
> 
Looks good, thanks!

> > +static int psr_cpu_prepare(unsigned int cpu)
> > +{
> > +    return cpu_prepare_work(cpu);
> > +}
> 
> What is this wrapper good for?
> 
Just keep the old codes.

> Jan

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