|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |