Hi Kuriakose,
Kuriakose Kuruvilla wrote:
> Hi Aubrey
>
> My suggestions after looking at the webrev...
>
> ==========
> SDM volume 3A (Section 13.2 P-STATE HARDWARE COORDINATION) says:
> Use CPUID to check the P-State hardware coordination feedback
> capability bit. CPUID.06H.ECX[Bit 0] = 1 indicates IA32_MPERF MSR
> and IA32_APERF MSR are present.
>
> Shouldn't we be checking this bit during initialization at
> turbo_supported()?
>
> ==========
turbo depends on MPERF/APERF. This is reduplicate if MPERF/APERF is only
used for turbo. Currently, no other module uses these two MSRs.
This is the only issue I can argue. The following suggestions are really good
to me,
especially the picky one, :)
Thanks for your comments and your time to review the patch, I'll update it soon.
-Aubrey
> powertop.h has a few variable definitions
> 208 turbo_info_t *cpu_turbo_info;
> ...
> 213 boolean_t g_turbo_supported;
> 214 double g_turbo_ratio;
>
> We usually define variables in dot.c files and have extern
> declarations in the dot.h files.
>
> ==========
> powertop.h
> 208 turbo_info_t *cpu_turbo_info;
>
> Since this is used only in turbo.c, can we move this to
> turbo.c and make
> it static?
>
> ==========
> Why define turbo_kstat_t and cpupm_turbo_info in speedstep.h
> if they are
> only used in speedstep.c?
>
> ==========
> There are a few places in turbo.c where we return positive values. In
> case of pt_turbo_stat_init(), we do not enable the turbo mode flag in
> the features variable. But in the case of pt_turbo_stat_collect()
> we error out only if we get negative return values. 239
> if (features & FEATURE_TURBO && 240
> pt_turbo_stat_collect() < 0) { 241
> exit(EXIT_FAILURE); 242 }
> Is this okay? Are we fine with show_cstates() displaying
> possibly stale
> data in this case?
>
> ==========
> In speedstep.c:speedstep_init(), is ms_vendor guaranteed to be
> initialized to NULL? If not, it should be set to NULL when turbo
> mode is not available.
>
> ==========
> speedstep.c
> 141 turbo_info->t_mcnt += rdmsr(IA32_MPERF_MSR);
> 142 turbo_info->t_acnt += rdmsr(IA32_APERF_MSR); ...
> 194 turbo_info->t_mcnt += rdmsr(IA32_MPERF_MSR);
> 195 turbo_info->t_acnt += rdmsr(IA32_APERF_MSR);
>
> ;) PICKY PICKY PICKY
> Since the relative values of these two MSRs are important, should we
> move the addition operations after both the rdmsr()s?
>
> /kuriakose
>
> Li, Aubrey wrote:
>> The prototype of turbo observability support is almost done. webrev
>> is here: http://cr.opensolaris.org/~aubrey/turbo-support/
>>
>> It includes both kernel part and application part.
>> The screenshot is here:
>> http://www.opensolaris.org/os/project/tesla/Work/Powertop/turbo.JPG
>>
>> Welcome any comments and suggestions!
>>
>> Thanks,
>> -Aubrey
>> _______________________________________________
>> tesla-dev mailing list
>> tesla-dev at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/tesla-dev
