Re: [Xen-devel] [PATCH v11 08/23] x86: refactor psr: L3 CAT: set value: implement framework.

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

On 17-05-30 08:32:59, Jan Beulich wrote:
> >>> On 03.05.17 at 10:44, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -118,11 +118,13 @@ static const struct feat_props {
> >   *             COS ID. Every entry of cos_ref corresponds to one COS ID.
> >   */
> >  struct psr_socket_info {
> > -    bool feat_init;
> > -    spinlock_t ref_lock;
> >      /* Feature array's index is 'enum psr_feat_type' which is same as 
> > 'props' */
> >      struct feat_node *features[PSR_SOCKET_FEAT_NUM];
> > +    bool feat_init;
> >      unsigned int cos_ref[MAX_COS_REG_CNT];
> > +    spinlock_t ref_lock;
> 
> This shuffling of fields seems unmotivated and is not being explained
> in the description.
> 
Per your comment in v10, such movement may avoid false cacheline conflicts.
The comment is below.
    Also please try to space apart the two locks, to avoid false cacheline
    conflicts (e.g. the new lock may well go immediately before the array
    it pairs with).

> > @@ -178,6 +180,10 @@ static void free_socket_resources(unsigned int socket)
> >      }
> >  
> >      info->feat_init = false;
> > +
> > +    memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int));
> > +
> > +    memset(info->dom_ids, 0, ((DOMID_IDLE + 1) + 7) / 8);
> 
> bitmap_clear()
> 
I searched the codes and found 'bitmap_clear' is only defined in tools/. There
is no such definition in hypervisor. So, I did not use it.

> > @@ -449,11 +455,19 @@ void psr_ctxt_switch_to(struct domain *d)
> >  
> >      /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. 
> > */
> >      if ( psra->cos_mask )
> > -        psr_assoc_cos(&reg,
> > -                      d->arch.psr_cos_ids ?
> > -                      
> > d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] :
> > -                      0,
> > -                      psra->cos_mask);
> > +    {
> > +        unsigned int socket = cpu_to_socket(smp_processor_id());
> > +        struct psr_socket_info *info = socket_info + socket;
> > +        unsigned int cos = 0;
> > +
> > +        if ( d->arch.psr_cos_ids )
> > +            cos = d->arch.psr_cos_ids[socket];
> > +
> > +        if ( unlikely(!test_bit(d->domain_id, info->dom_ids)) )
> > +            cos = 0;
> 
> I think a brief comment here would be helpful. I also think the two
> if()-s would better be combined (after all to initialize cos to zero
> above, so you simply need to avoid overwriting it in the first if().
> 
Ok, will add comment and merge the two if()-s.

> > @@ -529,6 +543,10 @@ int psr_get_val(struct domain *d, unsigned int socket,
> >      if ( !feat || !feat_props[feat_type] )
> >          return -ENOENT;
> >  
> > +    if ( !test_bit(d->domain_id, socket_info[socket].dom_ids) &&
> > +         d->arch.psr_cos_ids[socket] )
> > +        d->arch.psr_cos_ids[socket] = 0;
> 
> What is the right side of the && good for?
> 
The purpose is to avoid unnecessary restore action. But yes, this is a
redundant check.

> Also the latest here it is clear that "dom_ids" isn't the best choice
> for a name. What about "dom_set" or "domain_set"?
> 
I prefer 'dom_set'.

> > +/* The whole set process is protected by domctl_lock. */
> 
> This needs to be re-considered: We intend to incrementally
> reduce code ranges guarded by the domctl lock, and a good
> first step might be to stop introducing further dependencies on
> it in individual handlers. So the main question is: Do you need a
> global lock here at all, or would a per-domain one suffice? In
> the former case I think you should (re?)introduce your own,
> while in the latter case you could probably use domain_lock(),
> but since you don't require other per-domain activities to be
> synchronized, having your own private would perhaps be
> even better.
> 
The domctl_lock is used protect resources used in below functions:
1. psr_domain_init
2. psr_domain_free
3. psr_get_info
4. psr_get_val
5. psr_set_val

Per analysis, only 'd->arch.psr_cos_ids' should be protected in these
functions and psr_domain_init/free do not need lock because domain
cannot be scheduled when they are called.

So, I think 'domain_lock' is enough.

> > +int psr_set_val(struct domain *d, unsigned int socket,
> > +                uint64_t new_val, enum cbm_type type)
> > +{
> > +    unsigned int old_cos;
> > +    int cos, ret;
> > +    unsigned int *ref;
> > +    uint32_t *val_array, val;
> > +    struct psr_socket_info *info = get_socket_info(socket);
> > +    unsigned int array_len;
> > +    enum psr_feat_type feat_type;
> > +
> > +    if ( IS_ERR(info) )
> > +        return PTR_ERR(info);
> > +
> > +    if ( new_val != (uint32_t)new_val )
> > +        return -EINVAL;
> > +
> > +    val = new_val;
> 
> Please switch this and the prior if(), using val instead of the cast
> expression there.
> 
Got it.

> > +    feat_type = psr_cbm_type_to_feat_type(type);
> > +    if ( feat_type >= ARRAY_SIZE(info->features) ||
> > +         !info->features[feat_type] )
> > +        return -ENOENT;
> 
> Without seeing the code inside the functions you pass feat_type
> to below it's not really clear whether you wouldn't better use
> what is currently named psr_get_feat_and_type() here.
> 
'psr_get_feat_and_type' will be removed. So, I would like to keep codes here.
What is your opinion?

> > + free_array:
> > +    xfree(val_array);
> > +    return ret;
> > +
> > + unlock_free_array:
> > +    spin_unlock(&info->ref_lock);
> > +    xfree(val_array);
> > +    return ret;
> > +}
> 
> I'm sure I've said so before - please don't duplicate error paths like
> this. Here it's still easy to see all is fine, but what if each path gets
> two or three more thing added. Please chain them together via goto.
> 
To make things clear, I wrote below codes. How about them?
 unlock_free_array:
    spin_unlock(&info->ref_lock);

 free_array:
    xfree(val_array);
    return ret;

> >  /* 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;
> > +
> > +    ASSERT(socket_info);
> > +
> > +    if ( !d->arch.psr_cos_ids )
> > +        return;
> > +
> > +    /* Domain is destroied so its cos_ref should be decreased. */
> 
> destroyed
> 
> > +    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;
> 
> Does this "doesn't need to be handled" even extend to ...
> 
> > +        info = socket_info + socket;
> > +        spin_lock(&info->ref_lock);
> > +        ASSERT(info->cos_ref[cos]);
> > +        info->cos_ref[cos]--;
> > +        spin_unlock(&info->ref_lock);
> > +
> > +        clear_bit(d->domain_id, info->dom_ids);
> 
> ... this last part?
> 
Hmm, I should clear it no matter cos is 0 or not. Thanks!

> Jan

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