Re: [PATCH v11 04/23] x86: refactor psr: L3 CAT: implement main data structures, CPU init and free flows.

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-05-30 07:05:18, Jan Beulich wrote:
> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote:
> > +/*
> > + * PSR features are managed per socket. Below structure defines the members
> > + * used to manage these features.
> > + * ref_lock  - A lock to protect cos_ref.
> > + * features  - A feature node array used to manage all features enabled.
> > + * cos_ref   - A reference count array to record how many domains are using the
> > + *             COS ID. Every entry of cos_ref corresponds to one COS ID.
> > + */
> > +struct psr_socket_info {
> > +    bool feat_init;
> 
> The comment above lacks a line for this field.
> 
Will add it. Thanks!

> > +static void free_socket_resources(unsigned int socket)
> > +{
> > +    unsigned int i;
> > +    struct psr_socket_info *info = socket_info + socket;
> > +
> > +    if ( !info )
> > +        return;
> > +
> > +    /*
> > +     * Free resources of features. The global feature object, e.g. feat_l3_cat,
> > +     * may not be freed here if it is not added into array. It is simply being
> > +     * kept until the next CPU online attempt.
> > +     */
> > +    for ( i = 0; i < PSR_SOCKET_FEAT_NUM; i++ )
> > +    {
> > +        xfree(info->features[i]);
> > +        info->features[i] = NULL;
> > +    }
> 
> If you iterate over arrays, please use ARRAY_SIZE() as the loop
> boundary (and without knowing whether there are similar issues
> elsewhere in the series, please consider the comment given for
> all of it, just like any other comments usually are meant to apply
> to similar issues elsewhere).
> 
Ok, will check all loops.

> > +static void __init init_psr(void)
> > +{
> > +    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 "Failed to alloc socket_info!\n");
> 
> XENLOG_WARNING at least.
> 
Ok.

> >  static void psr_cpu_init(void)
> >  {
> > +    struct psr_socket_info *info;
> > +    unsigned int socket;
> > +    unsigned int cpu = smp_processor_id();
> > +    struct feat_node *feat;
> > +    struct cpuid_leaf regs;
> > +
> > +    if ( !psr_alloc_feat_enabled() || !boot_cpu_has(X86_FEATURE_PQE) )
> > +        goto assoc_init;
> > +
> > +    if ( boot_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT )
> > +    {
> > +        setup_clear_cpu_cap(X86_FEATURE_PQE);
> > +        goto assoc_init;
> > +    }
> > +
> > +    socket = cpu_to_socket(cpu);
> > +    info = socket_info + socket;
> > +    if ( info->feat_init )
> > +        goto assoc_init;
> > +
> > +    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;
> > +        feat_l3_cat = NULL;
> > +        l3_cat_props.type[0] = PSR_CBM_TYPE_L3;
> 
> Why is this not a static initializer? If it was, the whole l3_cat_props
> could be const.
> 
Good point. Will modify it.

> Jan

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