Re: [Xen-devel] [PATCH v8 08/24] x86: refactor psr: set value: implement framework.

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

On 17-03-08 09:07:10, Jan Beulich wrote:
> >>> On 15.02.17 at 09:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > As set value flow is the most complicated one in psr, it will be
> > divided to some patches to make things clearer. This patch
> > implements the set value framework to show a whole picture firstly.
> > 
> > It also changes domctl interface to make it more general.
> > 
> > To make the set value flow be general and can support multiple features
> > at same time, it includes below steps:
> > 1. Get COS ID of current domain using.
> 
> What is the "using" here supposed to mean?
> 
My meaning is to get the cos id that current domain is using. Sorry for this.
Will make it better.

> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -546,18 +546,214 @@ int psr_get_val(struct domain *d, unsigned int 
> > socket,
> >      return psr_get(socket, type, NULL, 0, d, val);
> >  }
> >  
> > -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 assemble_val_array(uint64_t *val,
> > +                              uint32_t array_len,
> > +                              const struct psr_socket_info *info,
> > +                              unsigned int old_cos)
> > +{
> > +    return -EINVAL;
> > +}
> > +
> > +static int set_new_val_to_array(uint64_t *val,
> 
> insert_new_val() ? And when talking about arrays, as indicated
> before, please use [] notation instead of pointers. This is
> particularly relevant when the function name suggests that it
> would be "val" which gets inserted, but aiui it is really ...
> 
> > +                                uint32_t array_len,
> > +                                const struct psr_socket_info *info,
> > +                                enum psr_feat_type feat_type,
> > +                                enum cbm_type type,
> > +                                uint64_t m)
> 
> ... "m". Therefore please also consider better naming of parameters.
> 
Sure, thanks!

> > +static int write_psr_msr(unsigned int socket, unsigned int cos,
> > +                         const uint64_t *val)
> > +{
> > +    return -ENOENT;
> > +}
> 
> Is this function intended you write just one MSR, or potentially many?
> In the latter case the name would perhaps better be "write_psr_msrs()".
> 
For one feature, it does set one MSR.

> > +int psr_set_val(struct domain *d, unsigned int socket,
> > +                uint64_t val, enum cbm_type type)
> > +{
> > +    unsigned int old_cos;
> > +    int cos, ret;
> > +    unsigned int *ref;
> > +    uint64_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];
> > +    if ( old_cos > MAX_COS_REG_CNT )
> > +        return -EOVERFLOW;
> 
> Doesn't this need to be >= ? And isn't this happening an indication
> of a bug, i.e. shouldn't there be an ASSERT_UNREACHABLE() ahead
> of the return?
> 
Sorry. This has been corrected in next version. Thanks!

> > +    ref = info->cos_ref;
> > +
> > +    /*
> > +     * Step 1:
> > +     * Assemle a value array to store all featues cos_reg_val[old_cos].
> 
> Assemble ... features ...
> 
Oh, sorry.

[...]
> > +    /*
> > +     * Step 2:
> > +     * Try to find if there is already a COS ID on which all features' 
> > values
> > +     * are same as the array. Then, we can reuse this COS ID.
> > +     */
> > +    cos = find_cos(val_array, array_len, feat_type, info);
> > +    if ( cos >= 0 )
> > +    {
> > +        if ( cos == old_cos )
> > +        {
> > +            spin_unlock(&info->ref_lock);
> > +            xfree(val_array);
> > +            return 0;
> > +        }
> 
> You could save a level of indentation if you inverted the outer if()'s
> condition and made the code above "else if".
> 
Will consider it.

> > +    }
> > +    else
> > +    {
> > +        /*
> > +         * Step 3:
> > +         * If fail to find, we need allocate a new COS ID.
> > +         * If multiple domains are using same COS ID, its ref is more
> > +         * than 1. That means we cannot free this COS to make current 
> > domain
> > +         * use it. Because other domains are using the value saved in the 
> > COS.
> > +         * Unless the ref is changed to 1 (mean only current domain is 
> > using
> > +         * it), we cannot allocate the COS ID to current domain.
> 
> I think I had been confused by this already before, and I continue to
> be: How could ref be "changed to 1" here, and then have said
> meaning? If you refer to the value after a possible decrement, the
> value then being 1 means there is another domain using it. Hence ...
> 
> > +         * So, only the COS ID which ref is 1 or 0 can be allocated.
> 
> ... I think this is not generally correct either: A COS with ref 1 can
> only be re-used it that's old_cos. In all other cases only ref 0 ones
> are candidates. But anyway I think the comment belongs into the
> function, which would then allow for seeing it be added along with
> the actual code, making it possible to check that both match up.
> 
Sorry, the expression is not good. In fact, only COS ID which ref is 1 or 0
can be allocated to current domain. If old_cos is not 0 and its ref==1 means
that only current domain is using this old_cos ID. So, this old_cos ID is
certainly can be reused by current domain. Ref==0 means there is no any domain
using this COS ID. So it can be used too.

I will polish the comments.

> > +         */
> > +        cos = pick_avail_cos(info, val_array, array_len, old_cos, 
> > feat_type);
> > +        if ( cos < 0 )
> > +        {
> > +            spin_unlock(&info->ref_lock);
> > +            xfree(val_array);
> > +            return cos;
> > +        }
> > +
> > +        /*
> > +         * Step 4:
> > +         * Write all features MSRs according to the COS ID.
> > +         */
> > +        ret = write_psr_msr(socket, cos, val_array);
> > +        if ( ret )
> > +        {
> > +            spin_unlock(&info->ref_lock);
> > +            xfree(val_array);
> > +            return ret;
> > +        }
> 
> These recurring error paths could certainly do with folding.
> 
Yes, Wei has suggested to use goto to handle them. This has been refined in
next version.

> > +    }
> > +
> > +    /*
> > +     * Step 5:
> > +     * Update ref according to COS ID.
> > +     */
> > +    ref[cos]++;
> > +    ASSERT(ref[cos] || cos == 0);
> 
>     ASSERT(!cos || ref[cos]);
>     ASSERT(!old_cos || ref[old_cos]);
> 
Ok, thanks!

> > +    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;
> 
> So the domain has not been paused, i.e. some of its vCPU-s may
> be running on other pCPU-s (including ones on the socket in
> question). How come it is safe to update this value here?
> 
This is a domctl operation. It is protected by domctl_lock which is locked in
do_domctl().

> >  /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */
> >  static void psr_free_cos(struct domain *d)
> >  {
> > -    if( !d->arch.psr_cos_ids )
> > +    unsigned int socket, cos;
> > +
> > +    if ( !d->arch.psr_cos_ids )
> >          return;
> 
> As in an earlier patch I've asked for this check to be removed, I
> think you will need to add a check on socket_info to be non-
> NULL somewhere in this function.
> 
Ok, will do it in the loop.

> > +    /* Domain is free so its cos_ref should be decreased. */
> 
> "Domain is free" ? DYM "is being destroyed"?
> 
Yes. 

> > +    for ( socket = 0; socket < nr_sockets; socket++ )
> > +    {
> > +        struct psr_socket_info *info;
> > +
> > +        /* cos 0 is default one which does not need be handled. */
> > +        if ( (cos = d->arch.psr_cos_ids[socket]) == 0 )
> > +            continue;
> > +
> > +        /*
> > +         * If domain uses other cos ids, all corresponding refs must have 
> > been
> > +         * increased 1 for this domain. So, we need decrease them.
> > +         */
> > +        info = socket_info + socket;
> > +        ASSERT(info->cos_ref[cos] || cos == 0);
> > +        spin_lock(&info->ref_lock);
> > +        info->cos_ref[cos]--;
> > +        spin_unlock(&info->ref_lock);
> 
> The ASSERT() is useful only inside the locked region.
> 
Ok, thanks!

> Jan

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