| Message ID | 1493801063-38513-9-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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. > @@ -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() > @@ -449,11 +455,19 @@ void psr_ctxt_switch_to(struct domain *d) > > /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ > if ( psra->cos_mask ) > - psr_assoc_cos(®, > - d->arch.psr_cos_ids ? > - d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > - 0, > - psra->cos_mask); > + { > + unsigned int socket = cpu_to_socket(smp_processor_id()); > + struct psr_socket_info *info = socket_info + socket; > + unsigned int cos = 0; > + > + if ( d->arch.psr_cos_ids ) > + cos = d->arch.psr_cos_ids[socket]; > + > + if ( unlikely(!test_bit(d->domain_id, info->dom_ids)) ) > + cos = 0; I think a brief comment here would be helpful. I also think the two if()-s would better be combined (after all to initialize cos to zero above, so you simply need to avoid overwriting it in the first if(). > @@ -529,6 +543,10 @@ int psr_get_val(struct domain *d, unsigned int socket, > if ( !feat || !feat_props[feat_type] ) > return -ENOENT; > > + if ( !test_bit(d->domain_id, socket_info[socket].dom_ids) && > + d->arch.psr_cos_ids[socket] ) > + d->arch.psr_cos_ids[socket] = 0; What is the right side of the && good for? Also the latest here it is clear that "dom_ids" isn't the best choice for a name. What about "dom_set" or "domain_set"? > +/* The whole set process is protected by domctl_lock. */ This needs to be re-considered: We intend to incrementally reduce code ranges guarded by the domctl lock, and a good first step might be to stop introducing further dependencies on it in individual handlers. So the main question is: Do you need a global lock here at all, or would a per-domain one suffice? In the former case I think you should (re?)introduce your own, while in the latter case you could probably use domain_lock(), but since you don't require other per-domain activities to be synchronized, having your own private would perhaps be even better. > +int psr_set_val(struct domain *d, unsigned int socket, > + uint64_t new_val, enum cbm_type type) > +{ > + unsigned int old_cos; > + int cos, ret; > + unsigned int *ref; > + uint32_t *val_array, val; > + 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); > + > + if ( new_val != (uint32_t)new_val ) > + return -EINVAL; > + > + val = new_val; Please switch this and the prior if(), using val instead of the cast expression there. > + 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. > + 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. > /* 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. */ destroyed > + 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; Does this "doesn't need to be handled" even extend to ... > + info = socket_info + socket; > + spin_lock(&info->ref_lock); > + ASSERT(info->cos_ref[cos]); > + info->cos_ref[cos]--; > + spin_unlock(&info->ref_lock); > + > + clear_bit(d->domain_id, info->dom_ids); ... this last part? Jan
On 17-05-30 08:32:59, Jan Beulich wrote: > >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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). > > @@ -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. > > @@ -449,11 +455,19 @@ void psr_ctxt_switch_to(struct domain *d) > > > > /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ > > if ( psra->cos_mask ) > > - psr_assoc_cos(®, > > - d->arch.psr_cos_ids ? > > - d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : > > - 0, > > - psra->cos_mask); > > + { > > + unsigned int socket = cpu_to_socket(smp_processor_id()); > > + struct psr_socket_info *info = socket_info + socket; > > + unsigned int cos = 0; > > + > > + if ( d->arch.psr_cos_ids ) > > + cos = d->arch.psr_cos_ids[socket]; > > + > > + if ( unlikely(!test_bit(d->domain_id, info->dom_ids)) ) > > + cos = 0; > > I think a brief comment here would be helpful. I also think the two > if()-s would better be combined (after all to initialize cos to zero > above, so you simply need to avoid overwriting it in the first if(). > Ok, will add comment and merge the two if()-s. > > @@ -529,6 +543,10 @@ int psr_get_val(struct domain *d, unsigned int socket, > > if ( !feat || !feat_props[feat_type] ) > > return -ENOENT; > > > > + if ( !test_bit(d->domain_id, socket_info[socket].dom_ids) && > > + d->arch.psr_cos_ids[socket] ) > > + d->arch.psr_cos_ids[socket] = 0; > > What is the right side of the && good for? > The purpose is to avoid unnecessary restore action. But yes, this is a redundant check. > Also the latest here it is clear that "dom_ids" isn't the best choice > for a name. What about "dom_set" or "domain_set"? > I prefer 'dom_set'. > > +/* The whole set process is protected by domctl_lock. */ > > This needs to be re-considered: We intend to incrementally > reduce code ranges guarded by the domctl lock, and a good > first step might be to stop introducing further dependencies on > it in individual handlers. So the main question is: Do you need a > global lock here at all, or would a per-domain one suffice? In > the former case I think you should (re?)introduce your own, > while in the latter case you could probably use domain_lock(), > but since you don't require other per-domain activities to be > synchronized, having your own private would perhaps be > even better. > The domctl_lock is used protect resources used in below functions: 1. psr_domain_init 2. psr_domain_free 3. psr_get_info 4. psr_get_val 5. psr_set_val Per analysis, only 'd->arch.psr_cos_ids' should be protected in these functions and psr_domain_init/free do not need lock because domain cannot be scheduled when they are called. So, I think 'domain_lock' is enough. > > +int psr_set_val(struct domain *d, unsigned int socket, > > + uint64_t new_val, enum cbm_type type) > > +{ > > + unsigned int old_cos; > > + int cos, ret; > > + unsigned int *ref; > > + uint32_t *val_array, val; > > + 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); > > + > > + if ( new_val != (uint32_t)new_val ) > > + return -EINVAL; > > + > > + val = new_val; > > Please switch this and the prior if(), using val instead of the cast > expression there. > Got it. > > + 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? > > + 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; > > /* 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. */ > > destroyed > > > + 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; > > Does this "doesn't need to be handled" even extend to ... > > > + info = socket_info + socket; > > + spin_lock(&info->ref_lock); > > + ASSERT(info->cos_ref[cos]); > > + info->cos_ref[cos]--; > > + spin_unlock(&info->ref_lock); > > + > > + clear_bit(d->domain_id, info->dom_ids); > > ... this last part? > Hmm, I should clear it no matter cos is 0 or not. Thanks! > Jan
>>> On 01.06.17 at 12:00, <yi.y.sun@linux.intel.com> wrote: > On 17-05-30 08:32:59, Jan Beulich wrote: >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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
On 17-06-01 04:45:58, Jan Beulich wrote: > >>> On 01.06.17 at 12:00, <yi.y.sun@linux.intel.com> wrote: > > On 17-05-30 08:32:59, Jan Beulich wrote: > >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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); }; > >> > + 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; > Jan
>>> On 02.06.17 at 04:49, <yi.y.sun@linux.intel.com> wrote: > On 17-06-01 04:45:58, Jan Beulich wrote: >> >>> On 01.06.17 at 12:00, <yi.y.sun@linux.intel.com> wrote: >> > On 17-05-30 08:32:59, Jan Beulich wrote: >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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
On 17-06-06 01:43:00, Jan Beulich wrote: > >>> On 02.06.17 at 04:49, <yi.y.sun@linux.intel.com> wrote: > > On 17-06-01 04:45:58, Jan Beulich wrote: > >> >>> On 01.06.17 at 12:00, <yi.y.sun@linux.intel.com> wrote: > >> > On 17-05-30 08:32:59, Jan Beulich wrote: > >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> 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. > Ok, I will be careful about this to put the new field to its final place when implement it. > >> >> > + 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. > How about this which can avoid a local variable? set_bit(d->domain_id, info->dom_ids); free_array: xfree(val_array); return ret; unlock_free_array: spin_unlock(&info->ref_lock); goto free_array; > Jan
>>> On 06.06.17 at 10:18, <yi.y.sun@linux.intel.com> wrote: > On 17-06-06 01:43:00, Jan Beulich wrote: >> >>> On 02.06.17 at 04:49, <yi.y.sun@linux.intel.com> wrote: >> > On 17-06-01 04:45:58, Jan Beulich wrote: >> >> >>> On 01.06.17 at 12:00, <yi.y.sun@linux.intel.com> wrote: >> >> > On 17-05-30 08:32:59, Jan Beulich wrote: >> >> >> >>> On 03.05.17 at 10:44, <yi.y.sun@linux.intel.com> wrote: >> >> >> > + 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. >> > How about this which can avoid a local variable? > > set_bit(d->domain_id, info->dom_ids); > > free_array: > xfree(val_array); > return ret; > > unlock_free_array: > spin_unlock(&info->ref_lock); > goto free_array; Yes, that's what I've been asking for. Jan
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 5b62c5c..124a1c6 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1411,21 +1411,21 @@ long arch_do_domctl( uint32_t val32; case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CBM: - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, - domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3); + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3); break; case XEN_DOMCTL_PSR_CAT_OP_SET_L3_CODE: - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, - domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_CODE); + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3_CODE); break; case XEN_DOMCTL_PSR_CAT_OP_SET_L3_DATA: - ret = psr_set_l3_cbm(d, domctl->u.psr_cat_op.target, - domctl->u.psr_cat_op.data, - PSR_CBM_TYPE_L3_DATA); + ret = psr_set_val(d, domctl->u.psr_cat_op.target, + domctl->u.psr_cat_op.data, + PSR_CBM_TYPE_L3_DATA); break; case XEN_DOMCTL_PSR_CAT_OP_GET_L3_CBM: diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index 1b781e1..7658786 100644 --- 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; + /* Every bit corresponds to a domain. Index is domain_id. */ + DECLARE_BITMAP(dom_ids, DOMID_IDLE + 1); }; struct psr_assoc { @@ -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); } static enum psr_feat_type psr_cbm_type_to_feat_type(enum cbm_type type) @@ -449,11 +455,19 @@ void psr_ctxt_switch_to(struct domain *d) /* IDLE domain's 'psr_cos_ids' is NULL so we set default value for it. */ if ( psra->cos_mask ) - psr_assoc_cos(®, - d->arch.psr_cos_ids ? - d->arch.psr_cos_ids[cpu_to_socket(smp_processor_id())] : - 0, - psra->cos_mask); + { + unsigned int socket = cpu_to_socket(smp_processor_id()); + struct psr_socket_info *info = socket_info + socket; + unsigned int cos = 0; + + if ( d->arch.psr_cos_ids ) + cos = d->arch.psr_cos_ids[socket]; + + if ( unlikely(!test_bit(d->domain_id, info->dom_ids)) ) + cos = 0; + + psr_assoc_cos(®, cos, psra->cos_mask); + } if ( reg != psra->val ) { @@ -529,6 +543,10 @@ int psr_get_val(struct domain *d, unsigned int socket, if ( !feat || !feat_props[feat_type] ) return -ENOENT; + if ( !test_bit(d->domain_id, socket_info[socket].dom_ids) && + d->arch.psr_cos_ids[socket] ) + d->arch.psr_cos_ids[socket] = 0; + cos = d->arch.psr_cos_ids[socket]; /* * If input cos exceeds current feature's cos_max, we should return its @@ -548,15 +566,218 @@ 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_into_array(uint32_t val[], + 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) +{ + return -ENOENT; +} + +static int pick_avail_cos(const struct psr_socket_info *info, + const uint32_t val[], unsigned int array_len, + unsigned int old_cos, + enum psr_feat_type feat_type) +{ + return -ENOENT; +} + +static int write_psr_msrs(unsigned int socket, unsigned int cos, + uint32_t val[], unsigned int array_len, + enum psr_feat_type feat_type) +{ + return -ENOENT; +} + +/* The whole set process is protected by domctl_lock. */ +int psr_set_val(struct domain *d, unsigned int socket, + uint64_t new_val, enum cbm_type type) +{ + unsigned int old_cos; + int cos, ret; + unsigned int *ref; + uint32_t *val_array, val; + 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); + + if ( new_val != (uint32_t)new_val ) + return -EINVAL; + + val = new_val; + + feat_type = psr_cbm_type_to_feat_type(type); + if ( feat_type >= ARRAY_SIZE(info->features) || + !info->features[feat_type] ) + return -ENOENT; + + /* + * Step 0: + * old_cos means the COS ID current domain is using. By default, it is 0. + * + * For every COS ID, there is a reference count to record how many domains + * are using the COS register corresponding to this COS ID. + * - If ref[old_cos] is 0, that means this COS is not used by any domain. + * - If ref[old_cos] is 1, that means this COS is only used by current + * domain. + * - If ref[old_cos] is more than 1, that mean multiple domains are using + * this COS. + */ + old_cos = d->arch.psr_cos_ids[socket]; + ASSERT(old_cos < MAX_COS_REG_CNT); + + ref = info->cos_ref; + + /* + * Step 1: + * Gather a value array to store all features cos_reg_val[old_cos]. + * And, set the input new val into array according to the feature's + * position in array. + */ + array_len = get_cos_num(info); + val_array = xzalloc_array(uint32_t, array_len); + if ( !val_array ) + return -ENOMEM; + + if ( (ret = gather_val_array(val_array, array_len, info, old_cos)) != 0 ) + goto free_array; + + if ( (ret = insert_val_into_array(val_array, array_len, info, + feat_type, type, val)) != 0 ) + goto free_array; + + spin_lock(&info->ref_lock); + + /* + * Step 2: + * Try to find if there is already a COS ID on which all features' values + * are same as the array. Then, we can reuse this COS ID. + */ + cos = find_cos(val_array, array_len, feat_type, info); + if ( cos == old_cos ) + { + ret = 0; + goto unlock_free_array; + } + + /* + * Step 3: + * If fail to find, we need pick an available COS ID. + * In fact, only COS ID which ref is 1 or 0 can be picked for current + * domain. If old_cos is not 0 and its ref==1, that means only current + * domain is using this old_cos ID. So, this old_cos ID certainly can + * be reused by current domain. Ref==0 means there is no any domain + * using this COS ID. So it can be used for current domain too. + */ + if ( cos < 0 ) + { + cos = pick_avail_cos(info, val_array, array_len, old_cos, feat_type); + if ( cos < 0 ) + { + ret = cos; + goto unlock_free_array; + } + + /* + * Step 4: + * Write the feature's MSRs according to the COS ID. + */ + ret = write_psr_msrs(socket, cos, val_array, array_len, feat_type); + if ( ret ) + goto unlock_free_array; + } + + /* + * 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; + + /* + * Step 7: + * Then, set the dom_ids bit which corresponds to domain_id to mark this + * domain has been set and the COS ID of the domain is valid. + */ + set_bit(d->domain_id, info->dom_ids); + + free_array: + xfree(val_array); + return ret; + + unlock_free_array: + spin_unlock(&info->ref_lock); + 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(info->cos_ref[cos]); + info->cos_ref[cos]--; + spin_unlock(&info->ref_lock); + + clear_bit(d->domain_id, info->dom_ids); + } + xfree(d->arch.psr_cos_ids); d->arch.psr_cos_ids = NULL; } diff --git a/xen/include/asm-x86/psr.h b/xen/include/asm-x86/psr.h index 7c6d38a..ae52d85 100644 --- a/xen/include/asm-x86/psr.h +++ b/xen/include/asm-x86/psr.h @@ -73,8 +73,8 @@ int psr_get_info(unsigned int socket, enum cbm_type type, uint32_t data[], unsigned int array_len); int psr_get_val(struct domain *d, unsigned int socket, uint32_t *val, enum cbm_type type); -int psr_set_l3_cbm(struct domain *d, unsigned int socket, - uint64_t cbm, enum cbm_type type); +int psr_set_val(struct domain *d, unsigned int socket, + uint64_t val, enum cbm_type type); int psr_domain_init(struct domain *d); void psr_domain_free(struct domain *d);
As set value flow is the most complicated one in psr, it will be divided to some patches to make things clearer. This patch implements the set value framework to show a whole picture firstly. It also changes domctl interface to make it more general. To make the set value flow be general and can support multiple features at same time, it includes below steps: 1. Get COS ID that current domain is using. 2. Gather a value array to store all features current value into it and replace the current value of the feature which is being set to the new input value. 3. Find if there is already a COS ID on which all features' values are same as the array. Then, we can reuse this COS ID. 4. If fail to find, we need pick an available COS ID. Only COS ID which ref is 0 or 1 can be picked. 5. Write the feature's MSRs according to the COS ID. 6. Update ref according to COS ID. 7. Save the COS ID into current domain's psr_cos_ids[socket] so that we can know which COS the domain is using on the socket. 8. Set dom_ids bit corresponding to the domain so that we can know the domain has been set and the COS ID of the domain is valid. So, some functions are abstracted and the callback functions will be implemented in next patches. Here is an example to understand the process. The CPU supports two featuers, e.g. L3 CAT and L2 CAT. User wants to set L3 CAT of Dom1 to 0x1ff. 1. At the initial time, the old_cos of Dom1 is 0. The COS registers values are below at this time. ------------------------------- | COS 0 | COS 1 | COS 2 | ... | ------------------------------- L3 CAT | 0x7ff | 0x7ff | 0x7ff | ... | ------------------------------- L2 CAT | 0xff | 0xff | 0xff | ... | ------------------------------- 2. Gather the value array and insert new value into it: val[0]: 0x1ff val[1]: 0xff 3. It cannot find a matching COS. 4. Pick COS 1 to store the value set. 5. Write the L3 CAT COS 1 registers. The COS registers values are changed to below now. ------------------------------- | COS 0 | COS 1 | COS 2 | ... | ------------------------------- L3 CAT | 0x7ff | 0x1ff | ... | ... | ------------------------------- L2 CAT | 0xff | 0xff | ... | ... | ------------------------------- 6. The ref[1] is increased to 1 because Dom1 is using it now. 7. Save 1 to Dom1's psr_cos_ids[socket]. 8. Set the bit in 'dom_ids[]'. Then, user wants to set L3 CAT of Dom2 to 0x1ff too. The old_cos of Dom2 is 0 too. Repeat above flow. The val array assembled is: val[0]: 0x1ff val[1]: 0xff So, it can find a matching COS, COS 1. Then, it can reuse COS 1 for Dom2. The ref[1] is increased to 2 now because both Dom1 and Dom2 are using this COS ID. Set 1 to Dom2's psr_cos_ids[socket]. There is one thing need to emphasize that we need restore domain's COS ID to 0 when socket is offline. Otherwise, a wrong COS ID will be used when the socket is online again. That may cause user see the wrong CBM shown. But it takes much time to iterate all domains to restore COS ID to 0. So, we define a 'dom_ids[]' to represents all domains, one bit corresponds to one domain. If the bit is 0 when entering 'psr_ctxt_switch_to', that means this is the first time the domain is switched to this socket or domain's COS ID has not been set since the socket is online. So, the COS ID set to ASSOC register on this socket should be default value, 0. If not, that means the domain's COS ID has been set when the socket was online. So, this COS ID is valid and we can directly use it. We restore the domain's COS ID to 0 if the bit corresponding to the domain is 0 but the domain's COS ID is not 0 when 'psr_get_val' is called. This can avoid CPU serialization if restoring action is exectued in 'psr_ctxt_switch_to'. Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- v11: - define 'dom_ids[]' and implement related flows. - restore domain cos id to 0 in 'psr_get_val'. - rename 'write_psr_msr' to 'write_psr_msrs' and change its parameters to handle value array the feature's all MSRs. - fix coding style issue. (suggested by Jan Beulich) - do not need check 'cos' in ASSERT. (suggested by Jan Beulich) - rename 'insert_val_to_array' to 'insert_val_into_array'. (suggested by Jan Beulich) - remove 'ref_lock' from parameter list in 'find_cos' and 'pick_avail_cos'. (suggested by Jan Beulich) - remove ASSERT check to 'ref_lock' in 'find_cos' and 'pick_avail_cos'. (suggested by Jan Beulich) - fix a bug for checking 'feat_type'. (suggested by Jan Beulich) - move 'free_array' label. (suggested by Jan Beulich) - modify comments and commit message. v10: - restore domain cos id to 0 when socket is offline. (suggested by Jan Beulich) - check 'psr_cat_op.data' to make sure only lower 32 bits are valid. (suggested by Jan Beulich) - remove unnecessary fixed width type of parameters and variables. (suggested by Jan Beulich) - rename 'insert_new_val_to_array' to 'insert_val_to_array'. (suggested by Jan Beulich) - input 'ref_lock' pointer into functions to check if it has been locked. (suggested by Jan Beulich) - add comment to declare the set process is protected by 'domctl_lock'. (suggested by Jan Beulich) - check 'feat_type'. (suggested by Jan Beulich) - remove 'feat_mask'. (suggested by Jan Beulich) - remove unnecessary criteria of ASSERT. (suggested by Jan Beulich) - adjust flow of 'psr_set_val' to avoid 'goto' for successful cases. (suggested by Jan Beulich) - use ASSERT to check 'socket_info' in 'psr_free_cos'. (suggested by Jan Beulich) - remove unnecessary comment in 'psr_free_cos'. (suggested by Jan Beulich) v9: - use goto style error handling in 'psr_set_val'. (suggested by Wei Liu) - use ASSERT for checking old_cos. (suggested by Wei Liu and Jan Beulich) - fix coding style issue. (suggested by Wei Liu) - rename 'assemble_val_array' to 'combine_val_array' in pervious patch. (suggested by Wei Liu) - use 'spin_is_locked' to check ref_lock. (suggested by Roger Pau) - add an input parameter 'array_len' for 'write_psr_msr'. - check 'socket_info' and 'psr_cos_ids' in this patch. (suggested by Jan Beulich) - modify patch title to indicate 'L3 CAT'. (suggested by Jan Beulich) - fix commit message words. (suggested by Jan Beulich) - change 'assemble_val_array' to 'gather_val_array'. (suggested by Jan Beulich) - change 'set_new_val_to_array' to 'insert_new_val_to_array'. (suggested by Jan Beulich) - change parameter 'm' of 'insert_new_val_to_array' to 'new_val'. (suggested by Jan Beulich) - change 'write_psr_msr' to 'write_psr_msrs'. (suggested by Jan Beulich) - correct comments. (suggested by Jan Beulich) - remove unnecessary comments. (suggested by Jan Beulich) - adjust conditions after 'find_cos' to save a level of indentation. (suggested by Jan Beulich) - add 'ASSERT(!old_cos || ref[old_cos])'. (suggested by Jan Beulich) - move ASSERT() check into locked region. (suggested by Jan Beulich) - replace parameter '*val' to 'val[]' in some functions. (suggested by Jan Beulich) - change 'write_psr_msr' parameters to prepare to only set one new value for one feature. (suggested by Jan Beulich) - changes about 'uint64_t' to 'uint32_t'. (suggested by Jan Beulich) - add explanation about context switch. (suggested by Jan Beulich) v5: - modify commit message. (suggested by Jan Beulich) - return an error for all helper functions in set flow. (suggested by Jan Beulich) - remove unnecessary cast. (suggested by Jan Beulich) - divide 'get_old_set_new' to two functions, 'assemble_val_array' and 'set_new_val_to_array'. (suggested by Jan Beulich) - modify comments. (suggested by Jan Beulich) - adjust code format. (suggested by Jan Beulich) - change 'alloc_new_cos' to 'pick_avail_cos' to make name accurate. (suggested by Jan Beulich) - check feature type when entering 'psr_set_val'. (suggested by Jan Beulich) - use ASSERT to check ref. (suggested by Jan Beulich) - rename 'dat[]' to 'data[]'. (suggested by Jan Beulich) v4: - create this patch to make codes easier to understand. (suggested by Jan Beulich) --- xen/arch/x86/domctl.c | 18 ++-- xen/arch/x86/psr.c | 239 ++++++++++++++++++++++++++++++++++++++++++++-- xen/include/asm-x86/psr.h | 4 +- 3 files changed, 241 insertions(+), 20 deletions(-)