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

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

On 17-02-26 17:41:08, Wei Liu wrote:
> On Wed, Feb 15, 2017 at 04:49:19PM +0800, Yi Sun wrote:
> > This patch implements the CPU init and free flow including L3 CAT
> > initialization and feature list free.
> > 
> > Signed-off-by: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> 
> Either you need to use a separate patch to move cpuid_count_leaf or you
> should state it is moved in the commit message.
> 
Thanks! I will add description in commit message.

> > +
> > +/* Common functions. */
> > +static void free_feature(struct psr_socket_info *info)
> > +{
> > +    struct feat_node *feat, *next;
> > +
> > +    if ( !info )
> > +        return;
> > +
> > +    /*
> > +     * Free resources of features. But we do not free global feature list
> > +     * entry, like feat_l3_cat. Although it may cause a few memory leak,
> > +     * it is OK simplify things.
> 
> I don't think it is OK to leak memory in the hypervisor in general.
> Please specify why it is OK in this particular case in the comment.
> 
In most cases, such global feature list entry will be added into feature list
so that it can be freed here.

In extreme case, e.g. socket 1 does not support L3 CAT. The feat_l3_cat
allocated in psr_cpu_prepare will not be released. But this is rare case.

Jan, Konrad and me disucssed this before. Per Jan's suggestion, we do not free
it.

> > +     */
> > +    list_for_each_entry_safe(feat, next, &info->feat_list, list)
> > +    {
> > +        if ( !feat )
> > +            return;
> > +
> > +        __clear_bit(feat->feature, &info->feat_mask);
> > +        list_del(&feat->list);
> > +        xfree(feat);
> > +    }
> > +}
> > +
> > -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 };
> > +
> > +    if ( !cpu_has(&current_cpu_data, X86_FEATURE_PQE) )
> > +        return;
> > +    else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT )
> > +    {
> > +        __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability);
> > +        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 )
> > +    {
> 
> You can move
> 
>    struct feat_node *feat
> 
> here.
> 
This variable will also be used by L2 CAT which codes exist in a different
branch. So, I declare it at the top of the function. Please refer below
patch:
[PATCH v8 17/24] x86: L2 CAT: implement CPU init and free flow.

> > +        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;
> > +        feat->ops = l3_cat_ops;
> > +
> > +        l3_cat_init_feature(regs, feat, info);
> > +    }
> > +}
> > +
> [...]
> >  
> > @@ -359,7 +528,7 @@ static int cpu_callback(
> >      switch ( action )
> >      {
> >      case CPU_UP_PREPARE:
> > -        rc = psr_cpu_prepare(cpu);
> > +        rc = psr_cpu_prepare();
> >          break;
> >      case CPU_STARTING:
> >          psr_cpu_init();
> > @@ -388,10 +557,14 @@ static int __init psr_presmp_init(void)
> >      if ( (opt_psr & PSR_CMT) && opt_rmid_max )
> >          init_psr_cmt(opt_rmid_max);
> >  
> > -    psr_cpu_prepare(0);
> > +    if ( opt_psr & PSR_CAT )
> > +        init_psr();
> > +
> > +    if ( psr_cpu_prepare() )
> > +        psr_free();
> >  
> >      psr_cpu_init();
> > -    if ( psr_cmt_enabled() )
> > +    if ( psr_cmt_enabled() || socket_info )
> 
> Why not have psr_cat_enabled() here?
> 
psr_cmt_enabled() returns true of false by checking if the global pointer
'psr_cmt' has been allocated or not. The 'psr_cmt' is also used in sysctl.c.
For allocation features, we have a similar global pointer 'socket_info'. But
it is only used in psr.c and all allocation features(CAT/CDP/MBA) use it. So
we directly use it in psr.c to check if related initialization has been done.

> Wei.

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