| Message ID | 20230501193034.88575-6-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Intel Hardware P-States (HWP) support | expand |
On 01.05.2023 21:30, Jason Andryuk wrote: > When using HWP, some of the returned data is not applicable. In that > case, we should just omit it to avoid confusing the user. So switch to > printing the base and turbo frequencies since those are relevant to HWP. > Similarly, stop printing the CPU frequencies since those do not apply. It vaguely feels like I have asked this before: Can you point me at a place in the SDM where it is said that CPUID 0x16's "Maximum Frequency" is the turbo frequency? Without such a reference I feel a little uneasy with ... > @@ -720,10 +721,15 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) > printf(" %d", p_cpufreq->affected_cpus[i]); > printf("\n"); > > - printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", > - p_cpufreq->cpuinfo_max_freq, > - p_cpufreq->cpuinfo_min_freq, > - p_cpufreq->cpuinfo_cur_freq); > + if ( internal ) > + printf("cpuinfo frequency : base [%u] turbo [%u]\n", > + p_cpufreq->cpuinfo_min_freq, > + p_cpufreq->cpuinfo_max_freq); ... calling it "turbo" (and not "max") here. Jan > + else > + printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", > + p_cpufreq->cpuinfo_max_freq, > + p_cpufreq->cpuinfo_min_freq, > + p_cpufreq->cpuinfo_cur_freq); > > printf("scaling_driver : %s\n", p_cpufreq->scaling_driver); >
On Thu, May 4, 2023 at 10:35 AM Jan Beulich <jbeulich@suse.com> wrote: > > On 01.05.2023 21:30, Jason Andryuk wrote: > > When using HWP, some of the returned data is not applicable. In that > > case, we should just omit it to avoid confusing the user. So switch to > > printing the base and turbo frequencies since those are relevant to HWP. > > Similarly, stop printing the CPU frequencies since those do not apply. > > It vaguely feels like I have asked this before: Can you point me at a > place in the SDM where it is said that CPUID 0x16's "Maximum Frequency" > is the turbo frequency? Without such a reference I feel a little uneasy > with ... I don't have a reference, but I found it empirically to match the "turbo" frequency. For an Intel® Core™ i7-10810U, https://ark.intel.com/content/www/us/en/ark/products/201888/intel-core-i710810u-processor-12m-cache-up-to-4-90-ghz.html Max Turbo Frequency 4.90 GHz # xenpm get-cpufreq-para cpu id : 0 affected_cpus : 0 cpuinfo frequency : base [1600000] turbo [4900000] Turbo has to be enabled to reach (close to) that frequency. From my cover letter: This is for a 10th gen 6-core 1600 MHz base 4900 MHZ max cpu. In the default balance mode, Turbo Boost doesn't exceed 4GHz. Tweaking the energy_perf preference with `xenpm set-cpufreq-hwp balance ene:64`, I've seen the CPU hit 4.7GHz before throttling down and bouncing around between 4.3 and 4.5 GHz. Curiously the other cores read ~4GHz when turbo boost takes affect. This was done after pinning all dom0 cores, and using taskset to pin to vCPU/pCPU 11 and running a bash tightloop. > > @@ -720,10 +721,15 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) > > printf(" %d", p_cpufreq->affected_cpus[i]); > > printf("\n"); > > > > - printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", > > - p_cpufreq->cpuinfo_max_freq, > > - p_cpufreq->cpuinfo_min_freq, > > - p_cpufreq->cpuinfo_cur_freq); > > + if ( internal ) > > + printf("cpuinfo frequency : base [%u] turbo [%u]\n", > > + p_cpufreq->cpuinfo_min_freq, > > + p_cpufreq->cpuinfo_max_freq); > > ... calling it "turbo" (and not "max") here. I'm fine with "max". I think I went with turbo since it's a value you cannot sustain but can only hit in short bursts. Regards, Jason
On 04.05.2023 19:00, Jason Andryuk wrote: > On Thu, May 4, 2023 at 10:35 AM Jan Beulich <jbeulich@suse.com> wrote: >> >> On 01.05.2023 21:30, Jason Andryuk wrote: >>> When using HWP, some of the returned data is not applicable. In that >>> case, we should just omit it to avoid confusing the user. So switch to >>> printing the base and turbo frequencies since those are relevant to HWP. >>> Similarly, stop printing the CPU frequencies since those do not apply. >> >> It vaguely feels like I have asked this before: Can you point me at a >> place in the SDM where it is said that CPUID 0x16's "Maximum Frequency" >> is the turbo frequency? Without such a reference I feel a little uneasy >> with ... > > I don't have a reference, but I found it empirically to match the > "turbo" frequency. > > For an Intel® Core™ i7-10810U, > https://ark.intel.com/content/www/us/en/ark/products/201888/intel-core-i710810u-processor-12m-cache-up-to-4-90-ghz.html > > Max Turbo Frequency 4.90 GHz > > # xenpm get-cpufreq-para > cpu id : 0 > affected_cpus : 0 > cpuinfo frequency : base [1600000] turbo [4900000] > > Turbo has to be enabled to reach (close to) that frequency. > > From my cover letter: > This is for a 10th gen 6-core 1600 MHz base 4900 MHZ max cpu. In the > default balance mode, Turbo Boost doesn't exceed 4GHz. Tweaking the > energy_perf preference with `xenpm set-cpufreq-hwp balance ene:64`, > I've seen the CPU hit 4.7GHz before throttling down and bouncing around > between 4.3 and 4.5 GHz. Curiously the other cores read ~4GHz when > turbo boost takes affect. This was done after pinning all dom0 cores, > and using taskset to pin to vCPU/pCPU 11 and running a bash tightloop. Right, but what matters for the longer term future is what gets committed (and the cover letter won't be). IOW ... >>> @@ -720,10 +721,15 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) >>> printf(" %d", p_cpufreq->affected_cpus[i]); >>> printf("\n"); >>> >>> - printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", >>> - p_cpufreq->cpuinfo_max_freq, >>> - p_cpufreq->cpuinfo_min_freq, >>> - p_cpufreq->cpuinfo_cur_freq); >>> + if ( internal ) >>> + printf("cpuinfo frequency : base [%u] turbo [%u]\n", >>> + p_cpufreq->cpuinfo_min_freq, >>> + p_cpufreq->cpuinfo_max_freq); >> >> ... calling it "turbo" (and not "max") here. > > I'm fine with "max". I think I went with turbo since it's a value you > cannot sustain but can only hit in short bursts. ... I don't mind you sticking to "turbo" as long as the description makes clear why that was chosen despite the SDM not naming it this way. Jan
On Fri, May 5, 2023 at 3:04 AM Jan Beulich <jbeulich@suse.com> wrote: > >>> @@ -720,10 +721,15 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) > >>> printf(" %d", p_cpufreq->affected_cpus[i]); > >>> printf("\n"); > >>> > >>> - printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", > >>> - p_cpufreq->cpuinfo_max_freq, > >>> - p_cpufreq->cpuinfo_min_freq, > >>> - p_cpufreq->cpuinfo_cur_freq); > >>> + if ( internal ) > >>> + printf("cpuinfo frequency : base [%u] turbo [%u]\n", > >>> + p_cpufreq->cpuinfo_min_freq, > >>> + p_cpufreq->cpuinfo_max_freq); > >> > >> ... calling it "turbo" (and not "max") here. > > > > I'm fine with "max". I think I went with turbo since it's a value you > > cannot sustain but can only hit in short bursts. > > ... I don't mind you sticking to "turbo" as long as the description makes > clear why that was chosen despite the SDM not naming it this way. I switched to "max" since as you point out that matches the SDM naming. Regards, Jason
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c index 1bb6187e56..ce8d7644d0 100644 --- a/tools/misc/xenpm.c +++ b/tools/misc/xenpm.c @@ -711,6 +711,7 @@ void start_gather_func(int argc, char *argv[]) /* print out parameters about cpu frequency */ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) { + bool internal = strstr(p_cpufreq->scaling_governor, XEN_HWP_GOVERNOR); int i; printf("cpu id : %d\n", cpuid); @@ -720,10 +721,15 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) printf(" %d", p_cpufreq->affected_cpus[i]); printf("\n"); - printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", - p_cpufreq->cpuinfo_max_freq, - p_cpufreq->cpuinfo_min_freq, - p_cpufreq->cpuinfo_cur_freq); + if ( internal ) + printf("cpuinfo frequency : base [%u] turbo [%u]\n", + p_cpufreq->cpuinfo_min_freq, + p_cpufreq->cpuinfo_max_freq); + else + printf("cpuinfo frequency : max [%u] min [%u] cur [%u]\n", + p_cpufreq->cpuinfo_max_freq, + p_cpufreq->cpuinfo_min_freq, + p_cpufreq->cpuinfo_cur_freq); printf("scaling_driver : %s\n", p_cpufreq->scaling_driver); @@ -750,19 +756,22 @@ static void print_cpufreq_para(int cpuid, struct xc_get_cpufreq_para *p_cpufreq) p_cpufreq->u.ondemand.up_threshold); } - printf("scaling_avail_freq :"); - for ( i = 0; i < p_cpufreq->freq_num; i++ ) - if ( p_cpufreq->scaling_available_frequencies[i] == - p_cpufreq->scaling_cur_freq ) - printf(" *%d", p_cpufreq->scaling_available_frequencies[i]); - else - printf(" %d", p_cpufreq->scaling_available_frequencies[i]); - printf("\n"); + if ( !internal ) + { + printf("scaling_avail_freq :"); + for ( i = 0; i < p_cpufreq->freq_num; i++ ) + if ( p_cpufreq->scaling_available_frequencies[i] == + p_cpufreq->scaling_cur_freq ) + printf(" *%d", p_cpufreq->scaling_available_frequencies[i]); + else + printf(" %d", p_cpufreq->scaling_available_frequencies[i]); + printf("\n"); - printf("scaling frequency : max [%u] min [%u] cur [%u]\n", - p_cpufreq->scaling_max_freq, - p_cpufreq->scaling_min_freq, - p_cpufreq->scaling_cur_freq); + printf("scaling frequency : max [%u] min [%u] cur [%u]\n", + p_cpufreq->scaling_max_freq, + p_cpufreq->scaling_min_freq, + p_cpufreq->scaling_cur_freq); + } printf("turbo mode : %s\n", p_cpufreq->turbo_enabled ? "enabled" : "disabled or n/a");
When using HWP, some of the returned data is not applicable. In that case, we should just omit it to avoid confusing the user. So switch to printing the base and turbo frequencies since those are relevant to HWP. Similarly, stop printing the CPU frequencies since those do not apply. Signed-off-by: Jason Andryuk <jandryuk@gmail.com> --- v2: Use full governor name XEN_HWP_GOVERNOR to change output Style fixes --- tools/misc/xenpm.c | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-)