Re: [Xen-devel] [PATCH v11 08/23] x86: refactor psr: L3 CAT: set value: implement framework.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -118,11 +118,13 @@ static const struct feat_props {
>   *             COS ID. Every entry of cos_ref corresponds to one COS ID.
>   */
>  struct psr_socket_info {
> -    bool feat_init;
> -    spinlock_t ref_lock;
>      /* Feature array's index is 'enum psr_feat_type' which is same as 
> 'props' */
>      struct feat_node *features[PSR_SOCKET_FEAT_NUM];
> +    bool feat_init;
>      unsigned int cos_ref[MAX_COS_REG_CNT];
> +    spinlock_t ref_lock;

This shuffling of fields seems unmotivated and is not being explained
in the description.

> @@ -178,6 +180,10 @@ static void free_socket_resources(unsigned int socket)
>      }
>  
>      info->feat_init = false;
> +
> +    memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int));
> +
> +    memset(info->dom_ids, 0, ((DOMID_IDLE + 1) + 7) / 8);

bitmap_clear()

> @@ -449,11 +455,19 @@ void psr_ctxt_switch_to(struct domain *d)
>  
>      /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */
>      if ( psra->cos_mask )
> -        psr_assoc_cos(&reg,
> -                      d->arch.psr_cos_ids ?
> -                      d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] 
> :
> -                      0,
> -                      psra->cos_mask);
> +    {
> +        unsigned int socket = cpu_to_socket(smp_processor_id());
> +        struct psr_socket_info *info = socket_info + socket;
> +        unsigned int cos = 0;
> +
> +        if ( d->arch.psr_cos_ids )
> +            cos = d->arch.psr_cos_ids[socket];
> +
> +        if ( unlikely(!test_bit(d->domain_id, info->dom_ids)) )
> +            cos = 0;

I think a brief comment here would be helpful. I also think the two
if()-s would better be combined (after all to initialize cos to zero
above, so you simply need to avoid overwriting it in the first if().

> @@ -529,6 +543,10 @@ int psr_get_val(struct domain *d, unsigned int socket,
>      if ( !feat || !feat_props[feat_type] )
>          return -ENOENT;
>  
> +    if ( !test_bit(d->domain_id, socket_info[socket].dom_ids) &&
> +         d->arch.psr_cos_ids[socket] )
> +        d->arch.psr_cos_ids[socket] = 0;

What is the right side of the && good for?

Also the latest here it is clear that "dom_ids" isn't the best choice
for a name. What about "dom_set" or "domain_set"?

> +/* The whole set process is protected by domctl_lock. */

This needs to be re-considered: We intend to incrementally
reduce code ranges guarded by the domctl lock, and a good
first step might be to stop introducing further dependencies on
it in individual handlers. So the main question is: Do you need a
global lock here at all, or would a per-domain one suffice? In
the former case I think you should (re?)introduce your own,
while in the latter case you could probably use domain_lock(),
but since you don't require other per-domain activities to be
synchronized, having your own private would perhaps be
even better.

> +int psr_set_val(struct domain *d, unsigned int socket,
> +                uint64_t new_val, enum cbm_type type)
> +{
> +    unsigned int old_cos;
> +    int cos, ret;
> +    unsigned int *ref;
> +    uint32_t *val_array, val;
> +    struct psr_socket_info *info = get_socket_info(socket);
> +    unsigned int array_len;
> +    enum psr_feat_type feat_type;
> +
> +    if ( IS_ERR(info) )
> +        return PTR_ERR(info);
> +
> +    if ( new_val != (uint32_t)new_val )
> +        return -EINVAL;
> +
> +    val = new_val;

Please switch this and the prior if(), using val instead of the cast
expression there.

> +    feat_type = psr_cbm_type_to_feat_type(type);
> +    if ( feat_type >= ARRAY_SIZE(info->features) ||
> +         !info->features[feat_type] )
> +        return -ENOENT;

Without seeing the code inside the functions you pass feat_type
to below it's not really clear whether you wouldn't better use
what is currently named psr_get_feat_and_type() here.

> + free_array:
> +    xfree(val_array);
> +    return ret;
> +
> + unlock_free_array:
> +    spin_unlock(&info->ref_lock);
> +    xfree(val_array);
> +    return ret;
> +}

I'm sure I've said so before - please don't duplicate error paths like
this. Here it's still easy to see all is fine, but what if each path gets
two or three more thing added. Please chain them together via goto.

>  /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */
>  static void psr_free_cos(struct domain *d)
>  {
> +    unsigned int socket, cos;
> +
> +    ASSERT(socket_info);
> +
> +    if ( !d->arch.psr_cos_ids )
> +        return;
> +
> +    /* Domain is destroied so its cos_ref should be decreased. */

destroyed

> +    for ( socket = 0; socket < nr_sockets; socket++ )
> +    {
> +        struct psr_socket_info *info;
> +
> +        /* cos 0 is default one which does not need be handled. */
> +        cos = d->arch.psr_cos_ids[socket];
> +        if ( cos == 0 )
> +            continue;

Does this "doesn't need to be handled" even extend to ...

> +        info = socket_info + socket;
> +        spin_lock(&info->ref_lock);
> +        ASSERT(info->cos_ref[cos]);
> +        info->cos_ref[cos]--;
> +        spin_unlock(&info->ref_lock);
> +
> +        clear_bit(d->domain_id, info->dom_ids);

... this last part?

Jan

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