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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Feb 27, 2017 at 02:42:31PM +0800, Yi Sun wrote:
> 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.

Then I would suggest you to not use "leak" in the comment. And put the
relevant bits from the discussion in the comment. Otherwise a drive-by
reviewer like me will call this out again. :-)

> 
> > > +     */
> > > +    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.

OK.

> 
> > > +        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.

The problem with using socket_info directly is that the name doesn't
tell you much. It doesn't carry specific semantics by itself. Wrapping
it inside an inline function with a proper name is much nicer. Also
there is the possibility that in the future you change the code to use
socket_info for a slightly different purpose or you want to expose it
outside of psr.c, you then need to retrospectively inspect all sites to
make sure you don't screw things up. IMHO using a psr_XXX_enabled like
psr_cmt_enabled is a small change with big benefit.

This is just a general suggestion. I don't feel too strongly about this.
If the maintainers are happy with the code as-is, you don't need to
change.

Wei.

> 
> > Wei.

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