[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v10 09/25] x86: refactor psr: L3 CAT: set value: implement framework.



On 17-04-11 09:01:53, Jan Beulich wrote:
> >>> On 01.04.17 at 15:53, <yi.y.sun@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/psr.c
> > +++ b/xen/arch/x86/psr.c
> > @@ -157,10 +157,26 @@ static void free_socket_resources(unsigned int socket)
> >  {
> >      unsigned int i;
> >      struct psr_socket_info *info = socket_info + socket;
> > +    struct domain *d;
> >  
> >      if ( !info )
> >          return;
> >  
> > +    /* Restore domain cos id to 0 when socket is offline. */
> > +    for_each_domain ( d )
> > +    {
> > +        unsigned int cos = d->arch.psr_cos_ids[socket];
> > +        if ( cos == 0 )
> 
> Blank line between declaration(s) and statement(s) please.
> 
Ok, will modify other places where have same issue.

> > +            continue;
> > +
> > +        spin_lock(&info->ref_lock);
> > +        ASSERT(!cos || info->cos_ref[cos]);
> 
> You've checked cos to be non-zero already above.
> 
Yep. Thanks!

> > +        info->cos_ref[cos]--;
> > +        spin_unlock(&info->ref_lock);
> > +
> > +        d->arch.psr_cos_ids[socket] = 0;
> > +    }
> 
> Overall, while you say in the revision log that this was a suggestion of
> mine, I don't recall any such (and I've just checked the v9 thread of
> this patch without finding anything), and hence it's not really clear to
> me why this is needed. After all you should be freeing info anyway

We discussed this in v9 05 patch. I paste it below for your convenience to
check.
[Sun Yi]:
  > So, you think the MSRs values may not be valid after such process and 
  > reloading (write MSRs to default value) is needed. If so, I would like 
  > to do more operations in 'free_feature()':
  > 1. Iterate all domains working on the offline socket to change
  >    'd->arch.psr_cos_ids[socket]' to COS 0, i.e restore it back to init
  >    status.
  > 2. Restore 'socket_info[socket].cos_ref[]' to all 0.
  > 
  > These can make the socket's info be totally restored back to init status.

[Jan]
  Yes, that's what I think is needed.

> (albeit I can't see this happening, which would look to be a bug in
> patch 5), so getting the refcounts adjusted seems pointless in any
> event. Whether d->arch.psr_cos_ids[socket] needs clearing I'm not

We only free resources in 'socket_info[socekt]' but do not free itself.
Below is how we allocate 'socket_info'. So, the 'socket_info[socekt]'
is not a pointer that can be directly freed.
  socket_info = xzalloc_array(struct psr_socket_info, nr_sockets);

That is the reason to reduce the 'info->cos_ref[cos]'.

> certain - this may indeed by unavoidable, to match up with
> psr_alloc_cos() using xzalloc.
> 
> Furthermore I'm not at all convinced this is appropriate to do in the
> context of a CPU_UP_CANCELED / CPU_DEAD notification: If you
> have a few thousand VMs, the loop above may take a while.
> 
Hmm, that may be a potential issue. I have two proposals below. Could you
please help to check which one you prefer? Or provide another solution?

1. Start a tasklet in free_socket_resources() to restore 'psr_cos_ids[socket]'
   of all domains. The action is protected by 'ref_lock' to avoid confliction
   in 'psr_set_val'. We can reduce 'info->cos_ref[cos]' in tasklet or memset
   the array to 0 in free_socket_resources().

2. Move 'psr_cos_ids[]' from 'domain' to 'psr_socket_info' and change index
   from 'socket' to 'domain_id'. So we keep all domains' COS IDs per socket
   and can memset the array to 0 when socket is offline. But here is an issue
   that we do not know how many members this array should have. I cannot find
   a macro something like 'DOMAIN_MAX_NUMBER'. So, I prefer to use reallocation
   in 'psr_alloc_cos' if the newly created domain's id is bigger than current
   array number.

> > @@ -574,15 +590,210 @@ int psr_get_val(struct domain *d, unsigned int 
> > socket,
> >      return 0;
> >  }
> >  
> > -int psr_set_l3_cbm(struct domain *d, unsigned int socket,
> > -                   uint64_t cbm, enum cbm_type type)
> > +/* Set value functions */
> > +static unsigned int get_cos_num(const struct psr_socket_info *info)
> >  {
> >      return 0;
> >  }
> >  
> > +static int gather_val_array(uint32_t val[],
> > +                            unsigned int array_len,
> > +                            const struct psr_socket_info *info,
> > +                            unsigned int old_cos)
> > +{
> > +    return -EINVAL;
> > +}
> > +
> > +static int insert_val_to_array(uint32_t val[],
> 
> As indicated before, I'm pretty convinced "into" would be more
> natural than "to".
> 
Got it. Thanks!

> > +                               unsigned int array_len,
> > +                               const struct psr_socket_info *info,
> > +                               enum psr_feat_type feat_type,
> > +                               enum cbm_type type,
> > +                               uint32_t new_val)
> > +{
> > +    return -EINVAL;
> > +}
> > +
> > +static int find_cos(const uint32_t val[], unsigned int array_len,
> > +                    enum psr_feat_type feat_type,
> > +                    const struct psr_socket_info *info,
> > +                    spinlock_t *ref_lock)
> 
> I don't think I did suggest adding another parameter here. The lock
> is accessible from info, isn't it? In which case, as I _did_ suggest,
> you should drop const from it instead. But ...
> 
> > +{
> > +    ASSERT(spin_is_locked(ref_lock));
> 
> ... the assertion seems rather pointless anyway with there just
> being a single caller, which very visibly acquires the lock up front.
> 
> > +static int pick_avail_cos(const struct psr_socket_info *info,
> > +                          spinlock_t *ref_lock,
> 
> Same here then.
> 
Ok, I will drop this assertion and the parameter 'ref_lock'.

> > +int psr_set_val(struct domain *d, unsigned int socket,
> > +                uint32_t val, enum cbm_type type)
> > +{
> > +    unsigned int old_cos;
> > +    int cos, ret;
> > +    unsigned int *ref;
> > +    uint32_t *val_array;
> > +    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);
> > +
> > +    feat_type = psr_cbm_type_to_feat_type(type);
> > +    if ( feat_type > ARRAY_SIZE(info->features) ||
> 
> I think I did point out the off-by-one mistake here in an earlier patch
> already.
> 
Sorry, I did not notice it.

[...]
> > +    /*
> > +     * Step 5:
> > +     * Find the COS ID (find_cos result is '>= 0' or an available COS ID is
> > +     * picked, then update ref according to COS ID.
> > +     */
> > +    ref[cos]++;
> > +    ASSERT(!cos || ref[cos]);
> > +    ASSERT(!old_cos || ref[old_cos]);
> > +    ref[old_cos]--;
> > +    spin_unlock(&info->ref_lock);
> > +
> > +    /*
> > +     * Step 6:
> > +     * Save the COS ID into current domain's psr_cos_ids[] so that we can 
> > know
> > +     * which COS the domain is using on the socket. One domain can only use
> > +     * one COS ID at same time on each socket.
> > +     */
> > +    d->arch.psr_cos_ids[socket] = cos;
> > +
> 
> Please put the "free_array" label here instead of duplicating the code
> below.
> 
Got it. Thx!

> > +    xfree(val_array);
> > +    return ret;
> > +
> > + 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. */
> > +    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;
> > +
> > +        info = socket_info + socket;
> > +        spin_lock(&info->ref_lock);
> > +        ASSERT(!cos || info->cos_ref[cos]);
> > +        info->cos_ref[cos]--;
> 
> This recurring pattern of assertion and decrement could surely be
> put in a helper function (and the for symmetry also for increment).
> 
Ok, but if we use memset to restore 'cos_ref[]' per above comment, I think it
is unnecessary.

> Jan

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.