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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> This patch implements the CPU init and free flow including L3 CAT
> initialization and feature list free.

I think from the final-effect-of-the-patch point of view the L3 CAT
aspect is more relevant, so that's what should appear in the title.

> @@ -136,11 +140,87 @@ struct psr_assoc {
>  
>  struct psr_cmt *__read_mostly psr_cmt;
>  
> +static struct psr_socket_info *__read_mostly socket_info;
> +
>  static unsigned int opt_psr;
>  static unsigned int __initdata opt_rmid_max = 255;
> +static unsigned int __read_mostly opt_cos_max = MAX_COS_REG_CNT;
>  static uint64_t rmid_mask;
>  static DEFINE_PER_CPU(struct psr_assoc, psr_assoc);
>  
> +/*
> + * Declare global feature list entry for every feature to facilitate the
> + * feature list creation. It will be allocated in psr_cpu_prepare() and
> + * inserted into feature list in cpu_init_work(). It is protected by
> + * cpu_add_remove_lock spinlock.
> + */
> +static struct feat_node *feat_l3_cat;

The comment is misleading imo: The only relevant aspect here is that
of when the allocation would need to happen vs. when it actually
happens (because putting it where we'd like it to be is not easily
possible). There's nothing list related here. I'd recommend simply
saying that we need a place to transiently store a spare node.

> +/* 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.
> +     */
> +    list_for_each_entry_safe(feat, next, &info->feat_list, list)
> +    {
> +        if ( !feat )
> +            return;

How would that happen? But well, this is going to go away anyway
with the move from list to array.

> @@ -335,18 +418,104 @@ void psr_domain_free(struct domain *d)
>      psr_free_rmid(d);
>  }
>  
> -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 };

I don't see you needing an initializer here at all, but if you really
want one for some reason, the same effect can be had with just
{}.

> +    if ( !cpu_has(&current_cpu_data, X86_FEATURE_PQE) )

Do you really mean to not universally check the global (boot CPU)
flag? I.e. possibly differing behavior on different CPUs?

> +        return;
> +    else if ( current_cpu_data.cpuid_level < PSR_CPUID_LEVEL_CAT )

Pointless "else".

> +    {
> +        __clear_bit(X86_FEATURE_PQE, current_cpu_data.x86_capability);

setup_clear_cpu_cap() if you use boot_cpu_has() above.

> +        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 )
> +    {
> +        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;

I don't think the comment is very useful: You've consumed the object,
so you simply should not leave a dangling pointer (or else you'd risk
multiple use).

>  static void psr_cpu_init(void)
>  {
> +    if ( socket_info )
> +        cpu_init_work();
> +
>      psr_assoc_init();
>  }
>  
>  static void psr_cpu_fini(unsigned int cpu)
>  {
> +    if ( socket_info )
> +        cpu_fini_work(cpu);
>      return;
>  }

Is it really useful to use another layer of helper functions here?

Jan

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