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

[Yi Sun <yi.y.sun@linux.intel.com>]

On 17-06-01 04:45:58, Jan Beulich wrote:
> >>> On 01.06.17 at 12:00, <yi.y.sun@linux.intel.com> wrote:
> > On 17-05-30 08:32:59, Jan Beulich wrote:
> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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).
> 
> Well - where is the second lock here?
> 
I thought 'feat_init' has same effect. But I should be wrong.

Then, I want to define the structure as below:

struct psr_socket_info {
    bool feat_init;
    /* Feature array's index is 'enum psr_feat_type' which is same as 'props' */
    struct feat_node *features[PSR_SOCKET_FEAT_NUM];
    spinlock_t ref_lock;
    unsigned int cos_ref[MAX_COS_REG_CNT];
    /* Every bit corresponds to a domain. Index is domain_id. */
    DECLARE_BITMAP(dom_ids, DOMID_IDLE + 1);
};

> >> > + 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;
> 
> I don't think that'll be okay for the case which previously fell
> through to free_array.
> 
I tried to understand your meaning. Do you mean below codes?

    set_bit(d->domain_id, info->dom_ids); //Success path.
    goto free_array;

 unlock_free_array:
    spin_unlock(&info->ref_lock);

 free_array:
    xfree(val_array);
    return ret;

> Jan

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