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

[Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>]

On 17-03-27 03:59:32, Jan Beulich wrote:
> >>> 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...;
> 
Thanks for the suggestion! I will check the 'data' for both get_val/set_val.

> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
[...]

> > +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)
> 
Hmm, will remove 'new'.
 
> > +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).
> 
Sorry for this. Will pass '&info->ref_lock' as a parameter into the function
to check it.

[...]
> > +    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).
> 
Yes, you are right. I will add comments.

> > +    /*
> > +     * 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).
> 
Sure, I will do it.

> > +     * 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".
> 
Ok.

> > +        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).
> 
Sorry, I don't understand your meaning here. DYM I should goto error handling
route? 'cos >= 0' means a valid cos id has been found. Then, we should do the
'cos_found' process to handle ref and set cos id into domain.

> > +    /*
> > +     * 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.
> 
I will try best to check entire series to keep it consistent.

> > +     * 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).
> 
Will correct all of them.

> > +    /*
> > +     * 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.
> 
DYM change 'free_array' to 'free'?

> >  /* 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.
> 
In normal case, 'psr_cos_ids' should not be NULL when entering this function.
So, I think ASSERT() is a good option. Thanks!

> > +    /* 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.
> 
After considering this comments again, I think the purpose of this piece of
codes has been explained outer the circle. So, I think this comment can be
removed. Is that OK for you?

> Jan

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