* [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info()
@ 2014-05-06 13:29 Jan Beulich
2014-05-06 13:49 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2014-05-06 13:29 UTC (permalink / raw)
To: xen-devel; +Cc: Boris Ostrovsky, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 4972 bytes --]
- some of the case statements are effectively or mostly special cases
of others, so there's no good reason not to share the code
- in the "get" function, a variable can be made case-wide instead of
having multiple instance of it (and those even with a pointless
initializer)
- minor formatting adjustments
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
v2: A few more formatting adjustments. Re-base on top of 82713ec8
("x86: use native RDTSC(P) execution when guest and host
frequencies are the same"), replacing that commit's outer ||
expression with a ?: one, thus better matching the comment (no
functional change as incarnation == 0 implies
d->arch.tsc_khz == cpu_khz).
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1794,39 +1794,34 @@ void tsc_get_info(struct domain *d, uint
switch ( *tsc_mode )
{
+ uint64_t tsc;
+
case TSC_MODE_NEVER_EMULATE:
- *elapsed_nsec = *gtsc_khz = 0;
+ *elapsed_nsec = *gtsc_khz = 0;
break;
- case TSC_MODE_ALWAYS_EMULATE:
- *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
- *gtsc_khz = d->arch.tsc_khz;
- break;
case TSC_MODE_DEFAULT:
if ( d->arch.vtsc )
{
+ case TSC_MODE_ALWAYS_EMULATE:
*elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
- *gtsc_khz = d->arch.tsc_khz;
- }
- else
- {
- uint64_t tsc = 0;
- rdtscll(tsc);
- *elapsed_nsec = scale_delta(tsc,&d->arch.vtsc_to_ns);
- *gtsc_khz = cpu_khz;
+ *gtsc_khz = d->arch.tsc_khz;
+ break;
}
+ rdtscll(tsc);
+ *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns);
+ *gtsc_khz = cpu_khz;
break;
case TSC_MODE_PVRDTSCP:
if ( d->arch.vtsc )
{
*elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
- *gtsc_khz = cpu_khz;
+ *gtsc_khz = cpu_khz;
}
else
{
- uint64_t tsc = 0;
rdtscll(tsc);
- *elapsed_nsec = (scale_delta(tsc,&d->arch.vtsc_to_ns) -
- d->arch.vtsc_offset);
+ *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) -
+ d->arch.vtsc_offset;
*gtsc_khz = 0; /* ignored by tsc_set_info */
}
break;
@@ -1883,38 +1878,32 @@ void tsc_set_info(struct domain *d,
switch ( d->arch.tsc_mode = tsc_mode )
{
- case TSC_MODE_NEVER_EMULATE:
- d->arch.vtsc = 0;
- break;
- case TSC_MODE_ALWAYS_EMULATE:
- d->arch.vtsc = 1;
- d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
- d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
- set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
- d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
- break;
case TSC_MODE_DEFAULT:
- d->arch.vtsc = 1;
+ case TSC_MODE_ALWAYS_EMULATE:
d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
- d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
- set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
+ d->arch.tsc_khz = gtsc_khz ?: cpu_khz;
+ set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000);
/*
- * Use native TSC if the host has safe TSC and:
+ * In default mode use native TSC if the host has safe TSC and:
* HVM/PVH: host and guest frequencies are the same (either
* "naturally" or via TSC scaling)
* PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz)
*/
- if ( host_tsc_is_safe() &&
- ((has_hvm_container_domain(d) &&
- (d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio)) ||
+ if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() &&
+ (has_hvm_container_domain(d) ?
+ d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio :
incarnation == 0) )
+ {
+ case TSC_MODE_NEVER_EMULATE:
d->arch.vtsc = 0;
- else
- d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
+ break;
+ }
+ d->arch.vtsc = 1;
+ d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
break;
case TSC_MODE_PVRDTSCP:
- d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
- host_tsc_is_safe() ? 0 : 1;
+ d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
+ host_tsc_is_safe() ? 0 : 1;
d->arch.tsc_khz = cpu_khz;
set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
[-- Attachment #2: x86-TSC-set-info-cleanup.patch --]
[-- Type: text/plain, Size: 5015 bytes --]
x86: reduce redundancy in tsc_[gs]et_info()
- some of the case statements are effectively or mostly special cases
of others, so there's no good reason not to share the code
- in the "get" function, a variable can be made case-wide instead of
having multiple instance of it (and those even with a pointless
initializer)
- minor formatting adjustments
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
v2: A few more formatting adjustments. Re-base on top of 82713ec8
("x86: use native RDTSC(P) execution when guest and host
frequencies are the same"), replacing that commit's outer ||
expression with a ?: one, thus better matching the comment (no
functional change as incarnation == 0 implies
d->arch.tsc_khz == cpu_khz).
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1794,39 +1794,34 @@ void tsc_get_info(struct domain *d, uint
switch ( *tsc_mode )
{
+ uint64_t tsc;
+
case TSC_MODE_NEVER_EMULATE:
- *elapsed_nsec = *gtsc_khz = 0;
+ *elapsed_nsec = *gtsc_khz = 0;
break;
- case TSC_MODE_ALWAYS_EMULATE:
- *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
- *gtsc_khz = d->arch.tsc_khz;
- break;
case TSC_MODE_DEFAULT:
if ( d->arch.vtsc )
{
+ case TSC_MODE_ALWAYS_EMULATE:
*elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
- *gtsc_khz = d->arch.tsc_khz;
- }
- else
- {
- uint64_t tsc = 0;
- rdtscll(tsc);
- *elapsed_nsec = scale_delta(tsc,&d->arch.vtsc_to_ns);
- *gtsc_khz = cpu_khz;
+ *gtsc_khz = d->arch.tsc_khz;
+ break;
}
+ rdtscll(tsc);
+ *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns);
+ *gtsc_khz = cpu_khz;
break;
case TSC_MODE_PVRDTSCP:
if ( d->arch.vtsc )
{
*elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
- *gtsc_khz = cpu_khz;
+ *gtsc_khz = cpu_khz;
}
else
{
- uint64_t tsc = 0;
rdtscll(tsc);
- *elapsed_nsec = (scale_delta(tsc,&d->arch.vtsc_to_ns) -
- d->arch.vtsc_offset);
+ *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) -
+ d->arch.vtsc_offset;
*gtsc_khz = 0; /* ignored by tsc_set_info */
}
break;
@@ -1883,38 +1878,32 @@ void tsc_set_info(struct domain *d,
switch ( d->arch.tsc_mode = tsc_mode )
{
- case TSC_MODE_NEVER_EMULATE:
- d->arch.vtsc = 0;
- break;
- case TSC_MODE_ALWAYS_EMULATE:
- d->arch.vtsc = 1;
- d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
- d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
- set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
- d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
- break;
case TSC_MODE_DEFAULT:
- d->arch.vtsc = 1;
+ case TSC_MODE_ALWAYS_EMULATE:
d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
- d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
- set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
+ d->arch.tsc_khz = gtsc_khz ?: cpu_khz;
+ set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000);
/*
- * Use native TSC if the host has safe TSC and:
+ * In default mode use native TSC if the host has safe TSC and:
* HVM/PVH: host and guest frequencies are the same (either
* "naturally" or via TSC scaling)
* PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz)
*/
- if ( host_tsc_is_safe() &&
- ((has_hvm_container_domain(d) &&
- (d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio)) ||
+ if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() &&
+ (has_hvm_container_domain(d) ?
+ d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio :
incarnation == 0) )
+ {
+ case TSC_MODE_NEVER_EMULATE:
d->arch.vtsc = 0;
- else
- d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
+ break;
+ }
+ d->arch.vtsc = 1;
+ d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
break;
case TSC_MODE_PVRDTSCP:
- d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
- host_tsc_is_safe() ? 0 : 1;
+ d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
+ host_tsc_is_safe() ? 0 : 1;
d->arch.tsc_khz = cpu_khz;
set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info()
2014-05-06 13:29 [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info() Jan Beulich
@ 2014-05-06 13:49 ` Andrew Cooper
2014-05-06 13:56 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Cooper @ 2014-05-06 13:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Boris Ostrovsky, Keir Fraser
[-- Attachment #1.1: Type: text/plain, Size: 5614 bytes --]
On 06/05/14 14:29, Jan Beulich wrote:
> - some of the case statements are effectively or mostly special cases
> of others, so there's no good reason not to share the code
> - in the "get" function, a variable can be made case-wide instead of
> having multiple instance of it (and those even with a pointless
> initializer)
> - minor formatting adjustments
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
With one query below,
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> v2: A few more formatting adjustments. Re-base on top of 82713ec8
> ("x86: use native RDTSC(P) execution when guest and host
> frequencies are the same"), replacing that commit's outer ||
> expression with a ?: one, thus better matching the comment (no
> functional change as incarnation == 0 implies
> d->arch.tsc_khz == cpu_khz).
>
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1794,39 +1794,34 @@ void tsc_get_info(struct domain *d, uint
>
> switch ( *tsc_mode )
> {
> + uint64_t tsc;
> +
> case TSC_MODE_NEVER_EMULATE:
> - *elapsed_nsec = *gtsc_khz = 0;
> + *elapsed_nsec = *gtsc_khz = 0;
> break;
> - case TSC_MODE_ALWAYS_EMULATE:
> - *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
> - *gtsc_khz = d->arch.tsc_khz;
> - break;
> case TSC_MODE_DEFAULT:
> if ( d->arch.vtsc )
> {
> + case TSC_MODE_ALWAYS_EMULATE:
> *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
> - *gtsc_khz = d->arch.tsc_khz;
> - }
> - else
> - {
> - uint64_t tsc = 0;
> - rdtscll(tsc);
> - *elapsed_nsec = scale_delta(tsc,&d->arch.vtsc_to_ns);
> - *gtsc_khz = cpu_khz;
> + *gtsc_khz = d->arch.tsc_khz;
> + break;
> }
> + rdtscll(tsc);
> + *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns);
> + *gtsc_khz = cpu_khz;
> break;
> case TSC_MODE_PVRDTSCP:
> if ( d->arch.vtsc )
> {
> *elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
> - *gtsc_khz = cpu_khz;
> + *gtsc_khz = cpu_khz;
> }
> else
> {
> - uint64_t tsc = 0;
> rdtscll(tsc);
> - *elapsed_nsec = (scale_delta(tsc,&d->arch.vtsc_to_ns) -
> - d->arch.vtsc_offset);
> + *elapsed_nsec = scale_delta(tsc, &d->arch.vtsc_to_ns) -
> + d->arch.vtsc_offset;
> *gtsc_khz = 0; /* ignored by tsc_set_info */
> }
> break;
> @@ -1883,38 +1878,32 @@ void tsc_set_info(struct domain *d,
>
> switch ( d->arch.tsc_mode = tsc_mode )
> {
> - case TSC_MODE_NEVER_EMULATE:
> - d->arch.vtsc = 0;
> - break;
> - case TSC_MODE_ALWAYS_EMULATE:
> - d->arch.vtsc = 1;
> - d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
> - d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
> - set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
> - d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
> - break;
> case TSC_MODE_DEFAULT:
> - d->arch.vtsc = 1;
> + case TSC_MODE_ALWAYS_EMULATE:
> d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
> - d->arch.tsc_khz = gtsc_khz ? gtsc_khz : cpu_khz;
> - set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
> + d->arch.tsc_khz = gtsc_khz ?: cpu_khz;
> + set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000);
> /*
> - * Use native TSC if the host has safe TSC and:
> + * In default mode use native TSC if the host has safe TSC and:
> * HVM/PVH: host and guest frequencies are the same (either
> * "naturally" or via TSC scaling)
> * PV: guest has not migrated yet (and thus arch.tsc_khz == cpu_khz)
> */
> - if ( host_tsc_is_safe() &&
> - ((has_hvm_container_domain(d) &&
> - (d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio)) ||
> + if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() &&
> + (has_hvm_container_domain(d) ?
> + d->arch.tsc_khz == cpu_khz || cpu_has_tsc_ratio :
> incarnation == 0) )
> + {
> + case TSC_MODE_NEVER_EMULATE:
> d->arch.vtsc = 0;
> - else
> - d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
> + break;
> + }
> + d->arch.vtsc = 1;
> + d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
> break;
> case TSC_MODE_PVRDTSCP:
> - d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
> - host_tsc_is_safe() ? 0 : 1;
> + d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
> + host_tsc_is_safe() ? 0 : 1;
Can this be reduced to boot_cpu_has(X86_FEATURE_RDTSCP) &&
!host_tsc_is_safe() ?
I believe this is the correct way around with the precedence between &&
and ?!, but it is far from clear. Alternatively, could some brackets be
introduced for clarity?
~Andrew
> d->arch.tsc_khz = cpu_khz;
> set_time_scale(&d->arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
> d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 6835 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info()
2014-05-06 13:49 ` Andrew Cooper
@ 2014-05-06 13:56 ` Jan Beulich
2014-05-06 14:04 ` Andrew Cooper
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2014-05-06 13:56 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Boris Ostrovsky, Keir Fraser
>>> On 06.05.14 at 15:49, <andrew.cooper3@citrix.com> wrote:
> On 06/05/14 14:29, Jan Beulich wrote:
>> case TSC_MODE_PVRDTSCP:
>> - d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
>> - host_tsc_is_safe() ? 0 : 1;
>> + d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
>> + host_tsc_is_safe() ? 0 : 1;
>
> Can this be reduced to boot_cpu_has(X86_FEATURE_RDTSCP) &&
> !host_tsc_is_safe() ?
>
> I believe this is the correct way around with the precedence between &&
> and ?!, but it is far from clear. Alternatively, could some brackets be
> introduced for clarity?
No, ?: has lower precedence than all binary operators other than
assignment ones and comma. But yes, this is ugly - I'll change it to
d->arch.vtsc = !(boot_cpu_has(X86_FEATURE_RDTSCP) && host_tsc_is_safe());
or, one character shorter,
d->arch.vtsc = !boot_cpu_has(X86_FEATURE_RDTSCP) || !host_tsc_is_safe();
(somehow I also managed to overlook the other double space in there).
Jan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info()
2014-05-06 13:56 ` Jan Beulich
@ 2014-05-06 14:04 ` Andrew Cooper
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2014-05-06 14:04 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Boris Ostrovsky, Keir Fraser
On 06/05/14 14:56, Jan Beulich wrote:
>>>> On 06.05.14 at 15:49, <andrew.cooper3@citrix.com> wrote:
>> On 06/05/14 14:29, Jan Beulich wrote:
>>> case TSC_MODE_PVRDTSCP:
>>> - d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
>>> - host_tsc_is_safe() ? 0 : 1;
>>> + d->arch.vtsc = boot_cpu_has(X86_FEATURE_RDTSCP) &&
>>> + host_tsc_is_safe() ? 0 : 1;
>> Can this be reduced to boot_cpu_has(X86_FEATURE_RDTSCP) &&
>> !host_tsc_is_safe() ?
>>
>> I believe this is the correct way around with the precedence between &&
>> and ?!, but it is far from clear. Alternatively, could some brackets be
>> introduced for clarity?
> No, ?: has lower precedence than all binary operators other than
> assignment ones and comma. But yes, this is ugly - I'll change it to
/Sigh - I specifically looked the precedence up, then mentally got it
wrong given the linebreak. It really is in need of some clarification.
>
> d->arch.vtsc = !(boot_cpu_has(X86_FEATURE_RDTSCP) && host_tsc_is_safe());
This was going to be my suggestion given the correct precedence.
~Andrew
>
> or, one character shorter,
>
> d->arch.vtsc = !boot_cpu_has(X86_FEATURE_RDTSCP) || !host_tsc_is_safe();
>
> (somehow I also managed to overlook the other double space in there).
>
> Jan
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-05-06 14:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-06 13:29 [PATCH v2] x86: reduce redundancy in tsc_[gs]et_info() Jan Beulich
2014-05-06 13:49 ` Andrew Cooper
2014-05-06 13:56 ` Jan Beulich
2014-05-06 14:04 ` Andrew Cooper
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).