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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.04.17 at 07:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-04-05 09:10:58, Jan Beulich wrote:
>> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > +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.

No, and this has been explained before: xfree() happily accepts
NULL pointers.

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

Excuse me, but if I suggested it there, how can it not be acceptable
to me?

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

This does not answer my question. Once again - why does this need
doing here explicitly, rather than relying on the needed values being
loaded when the first vCPU gets scheduled onto one of the pCPU-s
of this socket?

>> >  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'?

Well, while with the intended flag introduction this discussion is
mostly moot now - no, it's not the goto, it's the way of checking
done.

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

And (again mostly moot now) as expressed there, the further uses
appear to want checks different from the one here.

Jan

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