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-06 01:43:00, Jan Beulich wrote:
> >>> On 02.06.17 at 04:49, <yi.y.sun@linux.intel.com> wrote:
> > 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);
> > };
> 
> I've outlined my expectation to the ordering of fields before. The
> above broadly matches that, so would be fine. What I'd like to ask
> though is that fields don't get moved around without reason during
> the series. Insert new fields at their intended final place unless
> there's an actual reason to move them later.
> 
Ok, I will be careful about this to put the new field to its final
place when implement it.

> >> >> > + 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;
> 
> Coming close: Once again, using "goto" on error paths is half way
> acceptable to me, while using it anywhere else normally isn't.
> Hence you want the "unlock_free_array" path "goto free_array;"
> rather than the normal (success) one. Alternatively you might use
> a local variable to signal whether to release the lock.
> 
How about this which can avoid a local variable?

    set_bit(d->domain_id, info->dom_ids);

 free_array:
    xfree(val_array);
    return ret;

 unlock_free_array:
    spin_unlock(&info->ref_lock);
    goto free_array;

> Jan

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