| Message ID | 1492666704-6626-1-git-send-email-yi.y.sun@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 20.04.17 at 07:38, <yi.y.sun@linux.intel.com> wrote: > --- a/xen/arch/x86/psr.c > +++ b/xen/arch/x86/psr.c > @@ -125,6 +125,8 @@ struct feat_node { > uint32_t cos_reg_val[MAX_COS_REG_CNT]; > }; > > +#define PSR_DOM_IDS_NUM ((DOMID_IDLE + 1) / sizeof(uint32_t)) Instead of this, please use ... > @@ -134,9 +136,11 @@ struct feat_node { > * COS ID. Every entry of cos_ref corresponds to one COS ID. > */ > struct psr_socket_info { > - struct feat_node *features[PSR_SOCKET_MAX_FEAT]; > spinlock_t ref_lock; > + spinlock_t dom_ids_lock; > + struct feat_node *features[PSR_SOCKET_MAX_FEAT]; > unsigned int cos_ref[MAX_COS_REG_CNT]; > + uint32_t dom_ids[PSR_DOM_IDS_NUM]; ... DECLARE_BITMAP() here. 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). > @@ -221,12 +210,17 @@ static void free_socket_resources(unsigned int socket) > */ > for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > { > - if ( !info->features[i] ) > - continue; > - > xfree(info->features[i]); > info->features[i] = NULL; > } > + > + spin_lock(&info->ref_lock); > + memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int)); > + spin_unlock(&info->ref_lock); > + > + spin_lock_irqsave(&info->dom_ids_lock, flag); > + memset(info->dom_ids, 0, PSR_DOM_IDS_NUM * sizeof(uint32_t)); bitmap_clear() I'm also not convinced you need to acquire either of the two locks here - you're cleaning up the socket after all, so nothing can be running on it anymore. > @@ -682,9 +676,37 @@ void psr_ctxt_switch_to(struct domain *d) > psr_assoc_rmid(®, d->arch.psr_rmid); > > 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; > + > + if ( test_bit(d->domain_id, info->dom_ids) ) likely() > + goto set_assoc; I'm not convinced "goto" is reasonable to use here - this is not an error path. If you're afraid of the extra indentation level, make a helper function. > + spin_lock(&info->dom_ids_lock); > + > + int old_bit = test_and_set_bit(d->domain_id, info->dom_ids); Please don't mix declarations and statements. Also bool please, but then again the variable isn't really needed anyway. > + /* > + * If old_bit is 0, 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 domain's COS ID on this socket should > + * be default value, 0. If not, that means this socket has been offline > + * and the domain's COS ID has been set when the socket was online. So, > + * this COS ID is invalid and we have to restore it to 0. > + */ > + if ( d->arch.psr_cos_ids && > + old_bit == 0 && > + d->arch.psr_cos_ids[socket] != 0 ) Why don't you replicate the other two conditions in the if() trying to avoid taking the lock? (Especially if above you indeed intend to use a helper function, abstracting the full condition into another one would be very desirable.) > + d->arch.psr_cos_ids[socket] = 0; > + > + spin_unlock(&info->dom_ids_lock); And then, as a whole: As indicated before, ideally you'd keep this out of the context switch path altogether. What are the alternatives? > @@ -1310,7 +1283,10 @@ int psr_set_val(struct domain *d, unsigned int socket, > * which COS the domain is using on the socket. One domain can only use > * one COS ID at same time on each socket. > */ > + spin_lock_irqsave(&info->dom_ids_lock, flag); > d->arch.psr_cos_ids[socket] = cos; > + test_and_set_bit(d->domain_id, info->dom_ids); Why test_and_ when you don't use the result? Jan
On 17-04-26 04:04:15, Jan Beulich wrote: > >>> On 20.04.17 at 07:38, <yi.y.sun@linux.intel.com> wrote: > > --- a/xen/arch/x86/psr.c > > +++ b/xen/arch/x86/psr.c > > @@ -125,6 +125,8 @@ struct feat_node { > > uint32_t cos_reg_val[MAX_COS_REG_CNT]; > > }; > > > > +#define PSR_DOM_IDS_NUM ((DOMID_IDLE + 1) / sizeof(uint32_t)) > > Instead of this, please use ... > > > @@ -134,9 +136,11 @@ struct feat_node { > > * COS ID. Every entry of cos_ref corresponds to one COS ID. > > */ > > struct psr_socket_info { > > - struct feat_node *features[PSR_SOCKET_MAX_FEAT]; > > spinlock_t ref_lock; > > + spinlock_t dom_ids_lock; > > + struct feat_node *features[PSR_SOCKET_MAX_FEAT]; > > unsigned int cos_ref[MAX_COS_REG_CNT]; > > + uint32_t dom_ids[PSR_DOM_IDS_NUM]; > > ... DECLARE_BITMAP() here. > > 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). > Got it, thanks a lot for the suggestions! > > @@ -221,12 +210,17 @@ static void free_socket_resources(unsigned int socket) > > */ > > for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > > { > > - if ( !info->features[i] ) > > - continue; > > - > > xfree(info->features[i]); > > info->features[i] = NULL; > > } > > + > > + spin_lock(&info->ref_lock); > > + memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int)); > > + spin_unlock(&info->ref_lock); > > + > > + spin_lock_irqsave(&info->dom_ids_lock, flag); > > + memset(info->dom_ids, 0, PSR_DOM_IDS_NUM * sizeof(uint32_t)); > > bitmap_clear() > > I'm also not convinced you need to acquire either of the two locks > here - you're cleaning up the socket after all, so nothing can be > running on it anymore. > Can domain destroy happens at the same time when a socket is offline? > > @@ -682,9 +676,37 @@ void psr_ctxt_switch_to(struct domain *d) > > psr_assoc_rmid(®, d->arch.psr_rmid); > > > > 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; > > + > > + if ( test_bit(d->domain_id, info->dom_ids) ) > > likely() > Ok, thanks! > > + goto set_assoc; > > I'm not convinced "goto" is reasonable to use here - this is not an > error path. If you're afraid of the extra indentation level, make a > helper function. > Then, it seems a helper function is needed. > > + spin_lock(&info->dom_ids_lock); > > + > > + int old_bit = test_and_set_bit(d->domain_id, info->dom_ids); > > Please don't mix declarations and statements. Also bool please, > but then again the variable isn't really needed anyway. > Got it. Thanks! > > + /* > > + * If old_bit is 0, 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 domain's COS ID on this socket should > > + * be default value, 0. If not, that means this socket has been offline > > + * and the domain's COS ID has been set when the socket was online. So, > > + * this COS ID is invalid and we have to restore it to 0. > > + */ > > + if ( d->arch.psr_cos_ids && > > + old_bit == 0 && > > + d->arch.psr_cos_ids[socket] != 0 ) > > Why don't you replicate the other two conditions in the if() trying to > avoid taking the lock? (Especially if above you indeed intend to use > a helper function, abstracting the full condition into another one > would be very desirable.) > Ok, will move the two conditions to above 'if()', like below. if ( likely(test_bit(d->domain_id, info->dom_ids)) || !d->arch.psr_cos_ids || !d->arch.psr_cos_ids[socket] ) Accordingly, the later codes should be: spin_lock(&info->dom_ids_lock); set_bit(d->domain_id, info->dom_ids); d->arch.psr_cos_ids[socket] = 0; spin_unlock(&info->dom_ids_lock); > > + d->arch.psr_cos_ids[socket] = 0; > > + > > + spin_unlock(&info->dom_ids_lock); > > And then, as a whole: As indicated before, ideally you'd keep this > out of the context switch path altogether. What are the alternatives? > To restore domains' "psr_cos_ids[socket]" to default when socket offline happens, we have three time windows: 1. When socket is offline, in "free_socket_resources()"; 2. When socket is online, in "psr_cpu_init()"; 3. When context switch happens, in "psr_ctxt_switch_to()". Option 1 and 2 have same effect and option 1 is more natural than 2. So, we can do this restore action at "1" or "3". I have two alternatives below. Please help to check which you think is better: 1. The first version of the patch iterates valid domain list to restore them one by one. Per your comments, it may take much time. That is the reason I submitted this patch to spread out the restore action of all domains. If you think "psr_cos_ids[socket]" restore action happens in context switch path is not good, can we use a tasklet in "free_socket_resources()" to iterate the domain list and restore their "psr_cos_ids"? 2. Or, can we use a tasklet in "psr_ctxt_switch_to()" to do above work? The side effect is that the domain's COS ID used in this switch is not right. The valid COS ID may be set in next context switch. > > @@ -1310,7 +1283,10 @@ int psr_set_val(struct domain *d, unsigned int socket, > > * which COS the domain is using on the socket. One domain can only use > > * one COS ID at same time on each socket. > > */ > > + spin_lock_irqsave(&info->dom_ids_lock, flag); > > d->arch.psr_cos_ids[socket] = cos; > > + test_and_set_bit(d->domain_id, info->dom_ids); > > Why test_and_ when you don't use the result? > Sorry, set_bit() should be enough here. > Jan
>>> On 27.04.17 at 04:38, <yi.y.sun@linux.intel.com> wrote: > On 17-04-26 04:04:15, Jan Beulich wrote: >> >>> On 20.04.17 at 07:38, <yi.y.sun@linux.intel.com> wrote: >> > @@ -221,12 +210,17 @@ static void free_socket_resources(unsigned int socket) >> > */ >> > for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) >> > { >> > - if ( !info->features[i] ) >> > - continue; >> > - >> > xfree(info->features[i]); >> > info->features[i] = NULL; >> > } >> > + >> > + spin_lock(&info->ref_lock); >> > + memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int)); >> > + spin_unlock(&info->ref_lock); >> > + >> > + spin_lock_irqsave(&info->dom_ids_lock, flag); >> > + memset(info->dom_ids, 0, PSR_DOM_IDS_NUM * sizeof(uint32_t)); >> >> bitmap_clear() >> >> I'm also not convinced you need to acquire either of the two locks >> here - you're cleaning up the socket after all, so nothing can be >> running on it anymore. >> > Can domain destroy happens at the same time when a socket is offline? Well, yes and no - it depends on what path exactly you sit here. Large parts of CPU onlining/offlining happen in stop-machine context, which would exclude domain destruction going on in parallel. >> > + /* >> > + * If old_bit is 0, 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 domain's COS ID on this socket should >> > + * be default value, 0. If not, that means this socket has been offline >> > + * and the domain's COS ID has been set when the socket was online. So, >> > + * this COS ID is invalid and we have to restore it to 0. >> > + */ >> > + if ( d->arch.psr_cos_ids && >> > + old_bit == 0 && >> > + d->arch.psr_cos_ids[socket] != 0 ) >> >> Why don't you replicate the other two conditions in the if() trying to >> avoid taking the lock? (Especially if above you indeed intend to use >> a helper function, abstracting the full condition into another one >> would be very desirable.) >> > Ok, will move the two conditions to above 'if()', like below. > > if ( likely(test_bit(d->domain_id, info->dom_ids)) || > !d->arch.psr_cos_ids || > !d->arch.psr_cos_ids[socket] ) > > Accordingly, the later codes should be: > > spin_lock(&info->dom_ids_lock); > set_bit(d->domain_id, info->dom_ids); > d->arch.psr_cos_ids[socket] = 0; > spin_unlock(&info->dom_ids_lock); Then you didn't fully understand: The test_and_ portion _cannot_ be moved out of the locked region, but a simple test_bit() can be replicated prior to taking the lock. >> > + d->arch.psr_cos_ids[socket] = 0; >> > + >> > + spin_unlock(&info->dom_ids_lock); >> >> And then, as a whole: As indicated before, ideally you'd keep this >> out of the context switch path altogether. What are the alternatives? >> > To restore domains' "psr_cos_ids[socket]" to default when socket offline > happens, we have three time windows: > 1. When socket is offline, in "free_socket_resources()"; > 2. When socket is online, in "psr_cpu_init()"; > 3. When context switch happens, in "psr_ctxt_switch_to()". > > Option 1 and 2 have same effect and option 1 is more natural than 2. So, we can > do this restore action at "1" or "3". > > I have two alternatives below. Please help to check which you think is better: > 1. The first version of the patch iterates valid domain list to restore them one > by one. Per your comments, it may take much time. That is the reason I submitted > this patch to spread out the restore action of all domains. If you think > "psr_cos_ids[socket]" restore action happens in context switch path is not good, > can we use a tasklet in "free_socket_resources()" to iterate the domain list and > restore their "psr_cos_ids"? If that tasklet (a) doesn't again take overly long and (b) is guaranteed to finish before the same socket may come back online again, then yes. Otherwise both the iterate-over-all-domains and the in-context-switch approaches have downsides, but the latter would then seem preferable (because it only affects performance without risking the system's health). The question is whether some 3rd method can't be found. > 2. Or, can we use a tasklet in "psr_ctxt_switch_to()" to do above work? The side > effect is that the domain's COS ID used in this switch is not right. The valid > COS ID may be set in next context switch. I think this would complicate things while at the same time making them worse. Jan
On 17-04-27 00:48:43, Jan Beulich wrote: > >>> On 27.04.17 at 04:38, <yi.y.sun@linux.intel.com> wrote: > > On 17-04-26 04:04:15, Jan Beulich wrote: > >> >>> On 20.04.17 at 07:38, <yi.y.sun@linux.intel.com> wrote: > >> > @@ -221,12 +210,17 @@ static void free_socket_resources(unsigned int socket) > >> > */ > >> > for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) > >> > { > >> > - if ( !info->features[i] ) > >> > - continue; > >> > - > >> > xfree(info->features[i]); > >> > info->features[i] = NULL; > >> > } > >> > + > >> > + spin_lock(&info->ref_lock); > >> > + memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int)); > >> > + spin_unlock(&info->ref_lock); > >> > + > >> > + spin_lock_irqsave(&info->dom_ids_lock, flag); > >> > + memset(info->dom_ids, 0, PSR_DOM_IDS_NUM * sizeof(uint32_t)); > >> > >> bitmap_clear() > >> > >> I'm also not convinced you need to acquire either of the two locks > >> here - you're cleaning up the socket after all, so nothing can be > >> running on it anymore. > >> > > Can domain destroy happens at the same time when a socket is offline? > > Well, yes and no - it depends on what path exactly you sit here. > Large parts of CPU onlining/offlining happen in stop-machine > context, which would exclude domain destruction going on in > parallel. > The 'free_socket_resources' may be called when 'CPU_UP_CANCELED' or 'CPUU_DEAD' happens. For 'CPUU_DEAD', stop-machine is executed. But for 'CPU_UP_CANCELED', I cannot see this. 'CPU_UP_CANCELED' happens when cpu up fails and 'free_socket_resources' is called only when the cpu is the last one on socket. So for 'CPU_UP_CANCELED' case, 'free_socket_resources' should not be called. So, I think you are right and we can remove the spin_lock protections here. > >> > + /* > >> > + * If old_bit is 0, 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 domain's COS ID on this socket should > >> > + * be default value, 0. If not, that means this socket has been offline > >> > + * and the domain's COS ID has been set when the socket was online. So, > >> > + * this COS ID is invalid and we have to restore it to 0. > >> > + */ > >> > + if ( d->arch.psr_cos_ids && > >> > + old_bit == 0 && > >> > + d->arch.psr_cos_ids[socket] != 0 ) > >> > >> Why don't you replicate the other two conditions in the if() trying to > >> avoid taking the lock? (Especially if above you indeed intend to use > >> a helper function, abstracting the full condition into another one > >> would be very desirable.) > >> > > Ok, will move the two conditions to above 'if()', like below. > > > > if ( likely(test_bit(d->domain_id, info->dom_ids)) || > > !d->arch.psr_cos_ids || > > !d->arch.psr_cos_ids[socket] ) > > > > Accordingly, the later codes should be: > > > > spin_lock(&info->dom_ids_lock); > > set_bit(d->domain_id, info->dom_ids); > > d->arch.psr_cos_ids[socket] = 0; > > spin_unlock(&info->dom_ids_lock); > > Then you didn't fully understand: The test_and_ portion _cannot_ > be moved out of the locked region, but a simple test_bit() can be > replicated prior to taking the lock. > Oh, sorry, I should use test_and_ here to check the bit again which may be changed before the lock. Thanks for pointing it out! > >> > + d->arch.psr_cos_ids[socket] = 0; > >> > + > >> > + spin_unlock(&info->dom_ids_lock); > >> > >> And then, as a whole: As indicated before, ideally you'd keep this > >> out of the context switch path altogether. What are the alternatives? > >> > > To restore domains' "psr_cos_ids[socket]" to default when socket offline > > happens, we have three time windows: > > 1. When socket is offline, in "free_socket_resources()"; > > 2. When socket is online, in "psr_cpu_init()"; > > 3. When context switch happens, in "psr_ctxt_switch_to()". > > > > Option 1 and 2 have same effect and option 1 is more natural than 2. So, we can > > do this restore action at "1" or "3". > > > > I have two alternatives below. Please help to check which you think is better: > > 1. The first version of the patch iterates valid domain list to restore them one > > by one. Per your comments, it may take much time. That is the reason I submitted > > this patch to spread out the restore action of all domains. If you think > > "psr_cos_ids[socket]" restore action happens in context switch path is not good, > > can we use a tasklet in "free_socket_resources()" to iterate the domain list and > > restore their "psr_cos_ids"? > > If that tasklet (a) doesn't again take overly long and (b) is > guaranteed to finish before the same socket may come back online > again, then yes. Otherwise both the iterate-over-all-domains and > the in-context-switch approaches have downsides, but the latter > would then seem preferable (because it only affects performance > without risking the system's health). The question is whether some > 3rd method can't be found. > I have another solution now. We may move the psr_cos_ids[socket] restore action into 'psr_get_val' and only set the bit of 'dom_ids[]' in 'psr_set_val'. 1. When socket is offline, the dom_ids[] is cleared. 2. When socket is online, we have four places to use psr_cos_ids[socket]: a. psr_ctxt_, we can use test_bit() atomically check if the bit is set. If not, that means the d->arch.psr_cos_ids[socket] is invalid at this time. Then, we use 0 to set ASSOC register. But we don't restore psr_cos_ids here and do not set dom_ids[]. So, we do not need the spin_lock. b. psr_get_val, we use test_bit() to check if the bit is 0 and the d->arch.psr_cos_ids[socket] is not 0. If yes, that means this domain's cos id has not been restored yet. So we restore it to 0. c. psr_set_val, we set the bit in dom_ids[] and set d->arch.psr_cos_ids[socket]. As, psr_set_val cannot happen when psr_get_val is called, so no protection is needed. d. psr_free_cos, clear the bit and free d->arch.psr_cos_ids. This place cannot happen at the same time that the above three functions called. So, no protection needed. Per above analysis, we do not need lock protection. So, the CPU serialization issue can be solved. How do you think? > > 2. Or, can we use a tasklet in "psr_ctxt_switch_to()" to do above work? The side > > effect is that the domain's COS ID used in this switch is not right. The valid > > COS ID may be set in next context switch. > > I think this would complicate things while at the same time making > them worse. > > Jan > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
>>> On 27.04.17 at 11:30, <yi.y.sun@linux.intel.com> wrote: > I have another solution now. We may move the psr_cos_ids[socket] restore action > into 'psr_get_val' and only set the bit of 'dom_ids[]' in 'psr_set_val'. > 1. When socket is offline, the dom_ids[] is cleared. > 2. When socket is online, we have four places to use psr_cos_ids[socket]: > a. psr_ctxt_, we can use test_bit() atomically check if the bit is set. If > not, that means the d->arch.psr_cos_ids[socket] is invalid at this time. > Then, we use 0 to set ASSOC register. But we don't restore psr_cos_ids > here and do not set dom_ids[]. So, we do not need the spin_lock. > b. psr_get_val, we use test_bit() to check if the bit is 0 and the > d->arch.psr_cos_ids[socket] is not 0. If yes, that means this domain's > cos id has not been restored yet. So we restore it to 0. > c. psr_set_val, we set the bit in dom_ids[] and set > d->arch.psr_cos_ids[socket]. As, psr_set_val cannot happen when > psr_get_val is called, so no protection is needed. > d. psr_free_cos, clear the bit and free d->arch.psr_cos_ids. This place > cannot happen at the same time that the above three functions called. > So, no protection needed. > > Per above analysis, we do not need lock protection. So, the CPU serialization > issue can be solved. How do you think? Looks promising. Jan
On 17-04-27 03:39:02, Jan Beulich wrote: > >>> On 27.04.17 at 11:30, <yi.y.sun@linux.intel.com> wrote: > > I have another solution now. We may move the psr_cos_ids[socket] restore action > > into 'psr_get_val' and only set the bit of 'dom_ids[]' in 'psr_set_val'. > > 1. When socket is offline, the dom_ids[] is cleared. > > 2. When socket is online, we have four places to use psr_cos_ids[socket]: > > a. psr_ctxt_, we can use test_bit() atomically check if the bit is set. If > > not, that means the d->arch.psr_cos_ids[socket] is invalid at this time. > > Then, we use 0 to set ASSOC register. But we don't restore psr_cos_ids > > here and do not set dom_ids[]. So, we do not need the spin_lock. > > b. psr_get_val, we use test_bit() to check if the bit is 0 and the > > d->arch.psr_cos_ids[socket] is not 0. If yes, that means this domain's > > cos id has not been restored yet. So we restore it to 0. > > c. psr_set_val, we set the bit in dom_ids[] and set > > d->arch.psr_cos_ids[socket]. As, psr_set_val cannot happen when > > psr_get_val is called, so no protection is needed. > > d. psr_free_cos, clear the bit and free d->arch.psr_cos_ids. This place > > cannot happen at the same time that the above three functions called. > > So, no protection needed. > > > > Per above analysis, we do not need lock protection. So, the CPU serialization > > issue can be solved. How do you think? > > Looks promising. > Thank you! Then, it seems we have addressed most issues. I will prepare v11 and send it out in soon. > Jan
diff --git a/xen/arch/x86/psr.c b/xen/arch/x86/psr.c index a85ea99..7bc212f 100644 --- a/xen/arch/x86/psr.c +++ b/xen/arch/x86/psr.c @@ -125,6 +125,8 @@ struct feat_node { uint32_t cos_reg_val[MAX_COS_REG_CNT]; }; +#define PSR_DOM_IDS_NUM ((DOMID_IDLE + 1) / sizeof(uint32_t)) + /* * PSR features are managed per socket. Below structure defines the members * used to manage these features. @@ -134,9 +136,11 @@ struct feat_node { * COS ID. Every entry of cos_ref corresponds to one COS ID. */ struct psr_socket_info { - struct feat_node *features[PSR_SOCKET_MAX_FEAT]; spinlock_t ref_lock; + spinlock_t dom_ids_lock; + struct feat_node *features[PSR_SOCKET_MAX_FEAT]; unsigned int cos_ref[MAX_COS_REG_CNT]; + uint32_t dom_ids[PSR_DOM_IDS_NUM]; }; struct psr_assoc { @@ -194,26 +198,11 @@ static void free_socket_resources(unsigned int socket) { unsigned int i; struct psr_socket_info *info = socket_info + socket; - struct domain *d; + unsigned long flag; 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 ) - continue; - - spin_lock(&info->ref_lock); - ASSERT(!cos || info->cos_ref[cos]); - info->cos_ref[cos]--; - spin_unlock(&info->ref_lock); - - d->arch.psr_cos_ids[socket] = 0; - } - /* * Free resources of features. The global feature object, e.g. feat_l3_cat, * may not be freed here if it is not added into array. It is simply being @@ -221,12 +210,17 @@ static void free_socket_resources(unsigned int socket) */ for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) { - if ( !info->features[i] ) - continue; - xfree(info->features[i]); info->features[i] = NULL; } + + spin_lock(&info->ref_lock); + memset(info->cos_ref, 0, MAX_COS_REG_CNT * sizeof(unsigned int)); + spin_unlock(&info->ref_lock); + + spin_lock_irqsave(&info->dom_ids_lock, flag); + memset(info->dom_ids, 0, PSR_DOM_IDS_NUM * sizeof(uint32_t)); + spin_unlock_irqrestore(&info->dom_ids_lock, flag); } static bool feat_init_done(const struct psr_socket_info *info) @@ -682,9 +676,37 @@ void psr_ctxt_switch_to(struct domain *d) psr_assoc_rmid(®, d->arch.psr_rmid); 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; + + if ( test_bit(d->domain_id, info->dom_ids) ) + goto set_assoc; + + spin_lock(&info->dom_ids_lock); + + int old_bit = test_and_set_bit(d->domain_id, info->dom_ids); + + /* + * If old_bit is 0, 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 domain's COS ID on this socket should + * be default value, 0. If not, that means this socket has been offline + * and the domain's COS ID has been set when the socket was online. So, + * this COS ID is invalid and we have to restore it to 0. + */ + if ( d->arch.psr_cos_ids && + old_bit == 0 && + d->arch.psr_cos_ids[socket] != 0 ) + d->arch.psr_cos_ids[socket] = 0; + + spin_unlock(&info->dom_ids_lock); + + set_assoc: + psr_assoc_cos(®, + d->arch.psr_cos_ids ? d->arch.psr_cos_ids[socket] : 0, + psra->cos_mask); + } if ( reg != psra->val ) { @@ -1146,40 +1168,6 @@ static int write_psr_msr(unsigned int socket, unsigned int cos, return 0; } -static void restore_default_val(unsigned int socket, unsigned int cos, - enum psr_feat_type feat_type) -{ - unsigned int i, j; - uint32_t default_val; - const struct psr_socket_info *info = get_socket_info(socket); - - for ( i = 0; i < PSR_SOCKET_MAX_FEAT; i++ ) - { - const struct feat_node *feat = info->features[i]; - /* - * There are four judgements: - * 1. Input 'feat_type' is valid so we have to get feature according to - * it. If current feature type (i) does not match 'feat_type', we - * need skip it, so continue to check next feature. - * 2. Input 'feat_type' is 'PSR_SOCKET_MAX_FEAT' which means we should - * handle all features in this case. So, go to next loop. - * 3. Do not need restore the COS value back to default if cos_num is 1, - * e.g. L3 CAT. Because next value setting will overwrite it. - * 4. 'feat' we got is NULL, continue. - */ - if ( ( feat_type != PSR_SOCKET_MAX_FEAT && feat_type != i ) || - !feat || feat->props->cos_num == 1 ) - continue; - - for ( j = 0; j < feat->props->cos_num; j++ ) - { - feat->props->get_val(feat, 0, feat->props->type[j], &default_val); - write_psr_msr(socket, cos, default_val, - feat->props->type[j], i); - } - } -} - /* The whole set process is protected by domctl_lock. */ int psr_set_val(struct domain *d, unsigned int socket, uint32_t val, enum cbm_type type) @@ -1191,6 +1179,7 @@ int psr_set_val(struct domain *d, unsigned int socket, struct psr_socket_info *info = get_socket_info(socket); unsigned int array_len; enum psr_feat_type feat_type; + unsigned long flag; if ( IS_ERR(info) ) return PTR_ERR(info); @@ -1286,22 +1275,6 @@ int psr_set_val(struct domain *d, unsigned int socket, ASSERT(!cos || ref[cos]); ASSERT(!old_cos || ref[old_cos]); ref[old_cos]--; - - /* - * Step 6: - * For features, e.g. CDP, which cos_num is more than 1, we have to - * restore the old_cos value back to default when ref[old_cos] is 0. - * Otherwise, user will see wrong values when this COS ID is reused. E.g. - * user wants to set DATA to 0x3ff for a new domain. He hopes to see the - * DATA is set to 0x3ff and CODE should be the default value, 0x7ff. But - * if the COS ID picked for this action is the one that has been used by - * other domain and the CODE has been set to 0x1ff. Then, user will see - * DATA: 0x3ff, CODE: 0x1ff. So, we have to restore COS values for features - * using multiple COSs. - */ - if ( old_cos && !ref[old_cos] ) - restore_default_val(socket, old_cos, feat_type); - spin_unlock(&info->ref_lock); /* @@ -1310,7 +1283,10 @@ int psr_set_val(struct domain *d, unsigned int socket, * which COS the domain is using on the socket. One domain can only use * one COS ID at same time on each socket. */ + spin_lock_irqsave(&info->dom_ids_lock, flag); d->arch.psr_cos_ids[socket] = cos; + test_and_set_bit(d->domain_id, info->dom_ids); + spin_unlock_irqrestore(&info->dom_ids_lock, flag); xfree(val_array); return ret; @@ -1336,6 +1312,7 @@ static void psr_free_cos(struct domain *d) for ( socket = 0; socket < nr_sockets; socket++ ) { struct psr_socket_info *info; + unsigned long flag; /* cos 0 is default one which does not need be handled. */ cos = d->arch.psr_cos_ids[socket]; @@ -1346,14 +1323,11 @@ static void psr_free_cos(struct domain *d) spin_lock(&info->ref_lock); ASSERT(!cos || info->cos_ref[cos]); info->cos_ref[cos]--; - /* - * The 'cos_ref[cos]' of 'd' is 0 now so we need restore corresponding - * COS registers to default value. Because this case happens when a - * domain is destroied, we need restore all features. - */ - if ( !info->cos_ref[cos] ) - restore_default_val(socket, cos, PSR_SOCKET_MAX_FEAT); spin_unlock(&info->ref_lock); + + spin_lock_irqsave(&info->dom_ids_lock, flag); + clear_bit(d->domain_id, info->dom_ids); + spin_unlock_irqrestore(&info->dom_ids_lock, flag); } xfree(d->arch.psr_cos_ids); @@ -1453,6 +1427,7 @@ static void psr_cpu_init(void) goto assoc_init; spin_lock_init(&info->ref_lock); + spin_lock_init(&info->dom_ids_lock); cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 0, ®s); if ( regs.b & PSR_RESOURCE_TYPE_L3 )
Hi, Jan, Please help to review this patch. Thank you! Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com> --- xen/arch/x86/psr.c | 135 ++++++++++++++++++++++------------------------------- 1 file changed, 55 insertions(+), 80 deletions(-)