Re: [Xen-devel] [PATCH v11 04/23] x86: refactor psr: L3 CAT: implement main data structures, CPU init and free flows.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> +/*
> + * PSR features are managed per socket. Below structure defines the members
> + * used to manage these features.
> + * ref_lock  - A lock to protect cos_ref.
> + * features  - A feature node array used to manage all features enabled.
> + * cos_ref   - A reference count array to record how many domains are using 
> the
> + *             COS ID. Every entry of cos_ref corresponds to one COS ID.
> + */
> +struct psr_socket_info {
> +    bool feat_init;

The comment above lacks a line for this field.

> +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_FEAT_NUM; i++ )
> +    {
> +        xfree(info->features[i]);
> +        info->features[i] = NULL;
> +    }

If you iterate over arrays, please use ARRAY_SIZE() as the loop
boundary (and without knowing whether there are similar issues
elsewhere in the series, please consider the comment given for
all of it, just like any other comments usually are meant to apply
to similar issues elsewhere).

> +static void __init init_psr(void)
> +{
> +    if ( opt_cos_max < 1 )
> +    {
> +        printk(XENLOG_INFO "CAT: disabled, cos_max is too small\n");
> +        return;
> +    }
> +
> +    socket_info = xzalloc_array(struct psr_socket_info, nr_sockets);
> +
> +    if ( !socket_info )
> +    {
> +        printk(XENLOG_INFO "Failed to alloc socket_info!\n");

XENLOG_WARNING at least.

>  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 ( info->feat_init )
> +        goto assoc_init;
> +
> +    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;
> +        feat_l3_cat = NULL;
> +        l3_cat_props.type[0] = PSR_CBM_TYPE_L3;

Why is this not a static initializer? If it was, the whole l3_cat_props
could be const.

Jan


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