Re: [Xen-devel] [PATCH v9 09/25] x86: refactor psr: L3 CAT: set value: implement framework.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.17 at 12:07, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1437,21 +1437,21 @@ long arch_do_domctl(
>          switch ( domctl->u.psr_cat_op.cmd )
>          {
>          case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM:
> -            ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target,
> -                                 domctl->u.psr_cat_op.data,
> -                                 PSR_CBM_TYPE_L3);
> +            ret = psr_set_val(d, domctl->u.psr_cat_op.target,
> +                              (uint32_t)domctl->u.psr_cat_op.data,

The cast here is pointless, but - along the lines of the comment
on the earlier patch - indicates a problem: You silently ignore the
upper 32-bit the caller handed you. I think for forward
compatibility it would be better if you checked they're zero. And
in such a check you could then use a cast which I would not
grumble about:

    if ( domctl->u.psr_cat_op.data != (uint32_t)domctl->u.psr_cat_op.data )
        return -E...;

> --- a/xen/arch/x86/psr.c
> +++ b/xen/arch/x86/psr.c
> @@ -578,15 +578,203 @@ int psr_get_val(struct domain *d, unsigned int socket,
>      return 0;
>  }
>  
> -int psr_set_l3_cbm(struct domain *d, unsigned int socket,
> -                   uint64_t cbm, enum cbm_type type)
> +/* Set value functions */
> +static unsigned int get_cos_num(const struct psr_socket_info *info)
>  {
>      return 0;
>  }
>  
> +static int gather_val_array(uint32_t val[],
> +                            uint32_t array_len,

I think I've mentioned this before - please avoid fixed width types
where they're no needed. In the case here the array wants it, but
the length can easily be unsigned int.

> +                            const struct psr_socket_info *info,
> +                            unsigned int old_cos)
> +{
> +    return -EINVAL;
> +}
> +
> +static int insert_new_val_to_array(uint32_t val[],

insert_new_val_into_array()?

(and whether "new" needs to be part of the name is then even
questionable)

> +static int find_cos(const uint32_t val[], uint32_t array_len,
> +                    enum psr_feat_type feat_type,
> +                    const struct psr_socket_info *info)
> +{
> +    ASSERT(spin_is_locked((spinlock_t *)(&info->ref_lock)));

Excuse me, but no, this is another of those really bad casts. I
guess you've added it to deal with info being pointer-to-const.
In such a case you should instead drop the const, unless the
consuming function can be made take a pointer-to-const (which
isn't the case here, as check_lock() wants to write into the
structure).

> +int psr_set_val(struct domain *d, unsigned int socket,
> +                uint32_t val, enum cbm_type type)
> +{
> +    unsigned int old_cos;
> +    int cos, ret;
> +    unsigned int *ref;
> +    uint32_t *val_array;
> +    struct psr_socket_info *info = get_socket_info(socket);
> +    uint32_t array_len;
> +    enum psr_feat_type feat_type;
> +
> +    if ( IS_ERR(info) )
> +        return PTR_ERR(info);
> +
> +    feat_type = psr_cbm_type_to_feat_type(type);
> +    if ( !test_bit(feat_type, &info->feat_mask) )
> +        return -ENOENT;
> +
> +    /*
> +     * Step 0:
> +     * old_cos means the COS ID current domain is using. By default, it is 0.
> +     *
> +     * For every COS ID, there is a reference count to record how many 
> domains
> +     * are using the COS register corresponding to this COS ID.
> +     * - If ref[old_cos] is 0, that means this COS is not used by any domain.
> +     * - If ref[old_cos] is 1, that means this COS is only used by current
> +     *   domain.
> +     * - If ref[old_cos] is more than 1, that mean multiple domains are using
> +     *   this COS.
> +     */
> +    old_cos = d->arch.psr_cos_ids[socket];
> +    ASSERT(old_cos < MAX_COS_REG_CNT && old_cos >= 0);

The right side is always true and should hence be dropped.

> +    ref = info->cos_ref;
> +
> +    /*
> +     * Step 1:
> +     * Gather a value array to store all features cos_reg_val[old_cos].
> +     * And, set the input new val into array according to the feature's
> +     * position in array.
> +     */
> +    array_len = get_cos_num(info);
> +    val_array = xzalloc_array(uint32_t, array_len);
> +    if ( !val_array )
> +        return -ENOMEM;
> +
> +    if ( (ret = gather_val_array(val_array, array_len, info, old_cos)) != 0 )
> +        goto free_array;
> +
> +    if ( (ret = insert_new_val_to_array(val_array, array_len, info,
> +                                        feat_type, type, val)) != 0 )
> +        goto free_array;
> +
> +    spin_lock(&info->ref_lock);

Am I right in understanding that up to here operations are not
risking to race merely because they're inside the domctl lock? If so,
this needs to be spelled out, so we have a chance of noticing the
dependency when we break up that lock (which we have plans for
doing).

> +    /*
> +     * Step 2:
> +     * Try to find if there is already a COS ID on which all features' values

Btw., I think this spelling ("features' values") is the right one - please
go through and make it consistent everywhere, including in the patch
description(s).

> +     * are same as the array. Then, we can reuse this COS ID.
> +     */
> +    cos = find_cos(val_array, array_len, feat_type, info);
> +    if ( cos == old_cos )
> +    {
> +        ret = 0;
> +        goto unlock_free_array;
> +    }
> +    else if ( cos >= 0 )

Pointless "else".

> +        goto cos_found;

I think it would be better not to use goto here, other than for the
error handling parts (where I don't really like it either, but I do
accept it since others think that's the least ugly way).

> +    /*
> +     * Step 3:
> +     * If fail to find, we need pick an available COS ID.

I think you've corrected this somewhat strange wording at the start
of the comment here elsewhere already. I can only repeat that
respective comments given for one location should always be
extended to the entire series.

> +     * In fact, only COS ID which ref is 1 or 0 can be picked for current
> +     * domain. If old_cos is not 0 and its ref==1, that means only current
> +     * domain is using this old_cos ID. So, this old_cos ID certainly can
> +     * be reused by current domain. Ref==0 means there is no any domain
> +     * using this COS ID. So it can be used for current domain too.
> +     */
> +    cos = pick_avail_cos(info, val_array, array_len, old_cos, feat_type);
> +    if ( cos < 0 )
> +    {
> +        ret = cos;
> +        goto unlock_free_array;
> +    }
> +
> +    /*
> +     * Step 4:
> +     * Write all features MSRs according to the COS ID.
> +     */
> +    ret = write_psr_msr(socket, cos, val, type, feat_type);
> +    if ( ret )
> +        goto unlock_free_array;
> +
> +cos_found:

Style (also the other labels further down).

> +    /*
> +     * Step 5:
> +     * Update ref according to COS ID.
> +     */
> +    ref[cos]++;
> +    ASSERT(!cos || ref[cos]);
> +    ASSERT(!old_cos || ref[old_cos]);
> +    ref[old_cos]--;
> +    spin_unlock(&info->ref_lock);
> +
> +    /*
> +     * Step 6:
> +     * Save the COS ID into current domain's psr_cos_ids[] so that we can 
> know
> +     * which COS the domain is using on the socket. One domain can only use
> +     * one COS ID at same time on each socket.
> +     */
> +    d->arch.psr_cos_ids[socket] = cos;
> +    goto free_array;
> +
> +unlock_free_array:
> +    spin_unlock(&info->ref_lock);
> +free_array:
> +    xfree(val_array);
> +    return ret;
> +}

I think overall things would look better if the successful path was the
straight (goto-free) one.

>  /* 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;
> +
> +    if ( !socket_info || !d->arch.psr_cos_ids )
> +        return;

Can d->arch.psr_cos_ids be non-NULL when !socket_info? If not,
check only the former with an if(), and ASSERT() the latter.

> +    /* Domain is destroied so its cos_ref should be decreased. */
> +    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;
> +
> +        /*
> +         * If domain uses other cos ids, all corresponding refs must have 
> been
> +         * increased 1 for this domain. So, we need decrease them.

... by 1 ... need to ... I also question the presence of the word
"must" in here.

Jan

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