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