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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

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

bitmap_zero()

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

We'll see how it ends up being.

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

Jan


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