Re: [Xen-devel] [PATCH v10 03/25] x86: refactor psr: implement main data structures.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:

I was about to ack this, but there are a few more things which bother
me.

> ---
> v10:
>     - remove initialization for 'PSR_SOCKET_L3_CAT'.
>       (suggested by Jan Beulich)
>     - rename 'feat_ops' to 'feat_props'.
>       (suggested by Jan Beulich)
>     - move 'cbm_len' to 'feat_props' because it is feature independent so far.
>       (suggested by Jan Beulich)
>     - move 'cos_max' to 'feat_props' because it is feature independent.
>       (suggested by Jan Beulich)
>     - move 'cos_num' to 'feat_props' because it is feature independent.
>       (suggested by Jan Beulich)
>     - remove union 'info' and struct 'psr_cat_hw_info'.
>     - remove 'get_cos_max' from 'feat_props'.
>       (suggested by Jan Beulich)
>     - remove 'feat_mask' from 'psr_socket_info' because we can use 
> 'features[]'
>       to check if any feature is initialized.
>       (suggested by Jan Beulich)
>     - move 'ref_lock' above 'cos_ref'.
>       (suggested by Jan Beulich)

The movement done is fine for the moment, but it would have been
even better if you had moved it ahead of the other array too.

> +enum psr_feat_type {
> +    PSR_SOCKET_L3_CAT,
> +    PSR_SOCKET_L3_CDP,
> +    PSR_SOCKET_L2_CAT,
> +    PSR_SOCKET_MAX_FEAT,
> +};

It's not really logical to have the first three here - they should have
been added by their respective patches. Which gets me back to
the question of the usefulness of a patch like this one: Without the
following patches, everything being added here is unused. Iirc the
original version of this patch was quite a bit larger, better justifying
to break out all of this. The size it has shrunk to by now is a pretty
good indication that it should have been folded altogether.

Also I think we've had the discussion about the difference between
"max" and "num" already: The former normally indicates an inclusive
upper bound, while the latter would usually be an exclusive one.
Clearly the above naming doesn't match up with this.

> +/*
> + * This structure represents one feature.
> + * feat_props  - Feature properties, including operation callback functions
> +                 and feature common values.
> + * cos_reg_val - Array to store the values of COS registers. One entry stores
> + *               the value of one COS register.
> + *               For L3 CAT and L2 CAT, one entry corresponds to one COS_ID.
> + *               For CDP, two entries correspond to one COS_ID. E.g.
> + *               COS_ID=0 corresponds to cos_reg_val[0] (Data) and
> + *               cos_reg_val[1] (Code).
> + */
> +struct feat_node {
> +    /*
> +     * This structure defines feature operation callback functions. Every
> +     * feature enabled MUST implement such callback functions and register
> +     * them to props.
> +     *
> +     * Feature specific behaviors will be encapsulated into these callback
> +     * functions. Then, the main flows will not be changed when introducing
> +     * a new feature.
> +     *
> +     * Feature independent HW info and common values are also defined in it.
> +     */
> +    const struct feat_props {
> +        /*
> +         * cos_num, cos_max and cbm_len are common values for all features
> +         * so far.
> +         * cos_num - COS registers number that feature uses for one COS ID.
> +         *           It is defined in SDM.
> +         * cos_max - The max COS registers number got through CPUID.
> +         * cbm_len - The length of CBM got through CPUID.
> +         */
> +        unsigned int cos_num;
> +        unsigned int cos_max;
> +        unsigned int cbm_len;
> +    } *props;

Let's think the data arrangement changes done so far a little further:
Why do we need this pointer per-node (i.e. once per socket)? Now
that we have a well established order of features used to index
struct psr_socket_info's features[], why can't the same indexing be
used to obtain the props pointer from a static (single instance) array
of props pointers?

Otoh I'm not sure you really meant to move all three data members
into there, if you still have reason to believe that different sockets
may have different values for cos_max and/or cbm_len. I.e. these
might better be members of struct feat_node (just not placed in a
union, as you had them in v9).

Jan

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