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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 28.03.17 at 03:21, <yi.y.sun@linux.intel.com> wrote:
> On 17-03-27 03:59:32, Jan Beulich wrote:
>> >>> On 16.03.17 at 12:07, <yi.y.sun@linux.intel.com> 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.

There's no data to check on the "get" path - you return data there.

>> > +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.

That's suitable if you don't need "info" for anything else. Otherwise
- as said - you should drop const from info.

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

Using goto for error handling is acceptable, as said. But please use

    if ( cos < 0 )
    {
    }

followed by the code that's now after the cos_found label.

>> > +    /*
>> > +     * 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'?

My remark was not about label names, but about code flow: Please
use goto only for error paths, unless not using it on normal paths
means unacceptable resulting code (use your own judgment).

>> >  /* 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!

What does "normal" mean to you here? If, e.g. due to an error
during domain creation, we can come here with the pointer
being NULL, you must not ASSERT() but use if() instead. Please
note that my earlier comment did _not_ suggest to replace the
checking of d->arch.psr_cos_ids, but that of socket_info (as
long as the answer to the question is "no").

>> > +    /* 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?

If "outer the circle" means "outside of the loop", then yes, that earlier
comment should be sufficient on its own.

Jan

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