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

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

On 17-01-10 07:17:38, Jan Beulich wrote:
> > 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.
> > 2. Assemble a value array to store all features current value
> >    in it and replace the current value of the feature which is
> >    being set to the new input value.
> > 3. 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.
> > 4. If fail to find, we need allocate a new COS ID. Only COS ID which ref
> >    is 0 or 1 can be allocated.
> 
> Using "allocate" here in conjunction with ref count being 1 is a little
> misleading here - allocation would mean a fresh ID, whereas in the
> case of ref == 1 you mean to re-use the current one.
> 
Maybe 'pick an available COS ID' is more appropriate.

> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -513,18 +513,197 @@ int psr_get_val(struct domain *d, unsigned int 
> > socket,
> >      return -ENOENT;
> >  }
> >  
> > -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 get_old_set_new(uint64_t *val,
> > +                           uint32_t array_len,
> > +                           const struct psr_socket_info *info,
> > +                           unsigned int old_cos,
> > +                           enum cbm_type type,
> > +                           uint64_t m)
> > +{
> > +    return 0;
> > +}
> > +
> > +static int find_cos(const uint64_t *val, uint32_t array_len,
> > +                    enum cbm_type type,
> > +                    const struct psr_socket_info *info)
> > +{
> > +    return 0;
> > +}
> > +
> > +static int alloc_new_cos(const struct psr_socket_info *info,
> > +                         const uint64_t *val, uint32_t array_len,
> > +                         unsigned int old_cos,
> > +                         enum cbm_type type)
> > +{
> > +    return 0;
> > +}
> > +
> > +static int write_psr_msr(unsigned int socket, unsigned int cos,
> > +                         const uint64_t *val)
> > +{
> > +    return 0;
> > +}
> 
> I think all of the above functions should return an error as long as
> they're stubbed out, yet I don't think 0 means error in all cases (in
> particular a return value of plain int suggests 0 to mean success).
> 
Thanks, will consider the return values carefully.

> > +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;
> > +
> > +    if ( IS_ERR(info) )
> > +        return PTR_ERR(info);
> > +
> > +    /*
> > +     * 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;
> > +
> > +    ref = info->cos_ref;
> > +
> > +    /*
> > +     * Step 1:
> > +     * Assemle a value array to store all featues cos_reg_val[old_cos].
> > +     * And, set the input val into array according to the feature's
> > +     * position in array.
> > +     */
> > +    array_len = get_cos_num((const struct psr_socket_info *)info);
> 
> What is this cast doing here? (There are more of this kind below.)
> 
Hmm, I remember there may be warning without cast. Let me confirm it. If no
warning, will remove them.

> > +    val_array = xzalloc_array(uint64_t, array_len);
> > +    if ( !val_array )
> > +        return -ENOMEM;
> > +
> > +    if ( (ret = get_old_set_new(val_array, array_len,
> > +                                (const struct psr_socket_info *)info,
> > +                                old_cos, type, val)) != 0 )
> 
> Just like for earlier versions I continue to be unconvinced that
> the get-current-settings and the replace-target-value should be
> a single operation. In particular I'd expect the function to be able
> to store the target value, as long as the array entries are
> ordered in a suitable way.
> 
Ok, will split to two functions. One for getting old values of all features.
The other stores the target value into array according to features position
in feature list.

> > +    {
> > +        xfree(val_array);
> > +        return ret;
> > +    }
> > +
> > +    /*
> > +     * Lock here to make sure the ref is not changed during find and
> > +     * write process.
> > +     */
> > +    spin_lock(&info->ref_lock);
> > +
> > +    /*
> > +     * 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((const uint64_t *)val_array, array_len, type,
> > +                   (const struct psr_socket_info *)info);
> > +    if ( cos >= 0 )
> > +    {
> > +        if ( cos == old_cos )
> > +        {
> > +            spin_unlock(&info->ref_lock);
> > +            xfree(val_array);
> > +            return 0;
> > +        }
> > +    }
> > +    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.
> > +         * So, only the COS ID which ref is 1 or 0 can be allocated.
> > +         */
> 
> I suppose this comment will make more sense when further patches
> get applied, as so far there was no ref count check at all anywhere.
> 
Yes, the implementaion of alloc_new_cos() will show this. Comments here just
want to make readers be clear of the whole process.

> > +        cos = alloc_new_cos((const struct psr_socket_info *)info,
> > +                            (const uint64_t *)val_array, array_len,
> > +                            old_cos, 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, (const uint64_t *)val_array);
> > +        if ( ret )
> > +        {
> > +            spin_unlock(&info->ref_lock);
> > +            xfree(val_array);
> > +            return ret;
> > +        }
> > +    }
> > +
> > +    /*
> > +     * Step 5:
> > +     * Update ref according to COS ID.
> > +     */
> > +    ref[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.
> 
> To help readers, perhaps this last sentence should be completed with
> "... on each socket"?
> 
Thank you!

> > +     */
> > +    d->arch.psr_cos_ids[socket] = cos;
> > +    xfree(val_array);
> > +
> > +    return 0;
> > +}
> > +
> >  /* Called with domain lock held, no extra lock needed for 'psr_cos_ids' */
> >  static void psr_free_cos(struct domain *d)
> >  {
> > +    unsigned int socket;
> > +    unsigned int cos;
> 
> These could (and imo should) be joined together.
> 
Ok, thanks!

> > +    struct psr_socket_info *info;
> > +
> >      if( !d->arch.psr_cos_ids )
> >          return;
> >  
> > +    /* Domain is free so its cos_ref should be decreased. */
> > +    for( socket = 0; socket < nr_sockets; socket++ )
> 
> Missing blank (also in the if() above, not sure by which earlier patch
> that got introduced).
> 
Oh, sorry!

> > +    {
> > +        /* 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;
> > +        spin_lock(&info->ref_lock);
> > +        info->cos_ref[cos]--;
> 
> While likely also relevant in other places, this one in particular
> suggests that you should add ASSERT()s: Before decrements and
> after increments the ref count should not be zero.
> 
Ok, thanks, will check it.

> Also please move the declaration of at least "info" into the most
> narrow scope possible.
> 
Sure, will move it down.

> Jan

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