Re: [Xen-devel] [PATCH v10 05/25] x86: refactor psr: L3 CAT: implement CPU init and free flow.

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

On 17-04-05 09:10:58, Jan Beulich wrote:
> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > @@ -76,7 +79,7 @@ struct feat_node {
> >       *
> >       * Feature independent HW info and common values are also defined in 
> > it.
> >       */
> > -    const struct feat_props {
> > +    struct feat_props {
> 
> As said before, the const here should stay. The init-time writing
> to the structure can be done without going through this pointer.
> 
'feat_props' contains 'cos_max' and 'cbm_len' in this version. I have to assign
values to them in 'cat_init_feature'. So, I removed the 'const'.

Anyway, this structure will be moved out as a standalone struct to define a
static pointer array of 'props'. The 'cos_max' and 'cbm_len' will be moved to
'struct feat_node'.

> > +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_MAX_FEAT; i++ )
> > +    {
> > +        if ( !info->features[i] )
> > +            continue;
> > +
> > +        xfree(info->features[i]);
> > +        info->features[i] = NULL;
> 
> There's no need for the if() here. And I'm sure this was pointed out
> already (perhaps in a different context).
> 
There may be NULL member in features array. Features array contains all
features, including L3 CAT, CDP and L2 CAT. But on some machines, they
may only support L3 CAT but do not support CDP and L2 CAT. So, the features
array only has L3 CAT member in it and all other members are all NULL. That
is the reason we must check if the member is NULL or not.

> > +static bool feat_init_done(const struct psr_socket_info *info)
> > +{
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ )
> > +    {
> > +        if ( !info->features[i] )
> > +            continue;
> > +
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> 
> At the first glance this is a very strange function.
> 
I used 'feat_mask' before to check if any feature has been initialized.
Per your comment in later patch, I want to define a flag to represent it.
Is that acceptable to you?

> > +/* CAT common functions implementation. */
> > +static void cat_init_feature(const struct cpuid_leaf *regs,
> > +                             struct feat_node *feat,
> > +                             struct psr_socket_info *info,
> > +                             enum psr_feat_type type)
> > +{
> > +    unsigned int socket, i;
> > +
> > +    /* No valid value so do not enable feature. */
> > +    if ( !regs->a || !regs->d )
> > +        return;
> > +
> > +    feat->props->cbm_len = (regs->a & CAT_CBM_LEN_MASK) + 1;
> > +    feat->props->cos_max = min(opt_cos_max, regs->d & CAT_COS_MAX_MASK);
> > +
> > +    switch ( type )
> > +    {
> > +    case PSR_SOCKET_L3_CAT:
> > +        /* cos=0 is reserved as default cbm(all bits within cbm_len are 
> > 1). */
> > +        feat->cos_reg_val[0] = cat_default_val(feat->props->cbm_len);
> > +
> > +        /*
> > +         * To handle cpu offline and then online case, we need restore 
> > MSRs to
> > +         * default values.
> > +         */
> > +        for ( i = 1; i <= feat->props->cos_max; i++ )
> > +        {
> > +            wrmsrl(MSR_IA32_PSR_L3_MASK(i), feat->cos_reg_val[0]);
> > +            feat->cos_reg_val[i] = feat->cos_reg_val[0];
> > +        }
> 
> I continue to have difficulty with this: Why is offline-then-online
> any different from first-time-online? Why wouldn't setting the

May remove this comment. Per current codes, the MSRs are written to default
values no matter first time or not.

> registers to their intended values not be taken care of by
> context switch code, once vCPU-s get scheduled onto the newly
> onlined CPU?
> 
cat_init_feature is only called when the first CPU on a socket is online.
The MSRs to set are per socket. So, we only need set it once when socket
is online.

> > +        break;
> > +
> > +    default:
> > +        return;
> > +    }
> > +
> > +    /* Add this feature into array. */
> > +    info->features[type] = feat;
> > +
> > +    socket = cpu_to_socket(smp_processor_id());
> 
> No need for this variable, and definitely no need to do the
> assignment ahead of ...
> > +    if ( !opt_cpu_info )
> > +        return;
> 
> ... this.

Ok, will remove socket to directly use 'cpu_to_socket(smp_processor_id())'.

> 
> >  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 ( feat_init_done(info) )
> > +        goto assoc_init;
> 
> Hmm, so you bail here if any of the features was already set up.
> But you don't bail if none of the features were available as the
> reason for the setup not having been done before. I think this
> can be solved in a better way once we have the static props
> array: You need to do anything here only if the props slot is not
> NULL, but the feature slot is NULL.
> 
As above comment, this check will be changed to check a flag if you have
no opinion. But, what is your conern here? Do you mind the 'goto'?

> In any event you intentions are likely easier to understand for
> a reader if this single-use function was inlined here.
> 
As you have observed, this 'feat_init_done' is used many times in later
patches.

> Jan

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