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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.06.17 at 04:49, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> On 17-06-01 04:45:58, Jan Beulich wrote:
>> >>> On 01.06.17 at 12:00, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
>> > 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).
>> 
>> 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.

>> >> > + 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.

Jan


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