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
Hi Aubrey, here are my comments.
usr/src/cmd/powertop/turbo.c
- pt_turbo_stat_collect(void): looks like you'll leak memory if turbo
is not supported (i.e. calloc after checking if it's supported)
- since this routines is called at every interval, wouldn't it make
sense to use have a turbo_info_t t_new, as opposed to allocating/freeing
it all the time?
usr/src/cmd/powertop/powertop.c
- I'd suggest clearing g_turbo_supported around lines 80-83, before we
start initializing everything else.
usr/src/uts/i86pc/os/cpupm/speedstep.c
- the new routines are not prefixed with speedstep_. So I'd suggest
either adding that or creating a standard prefix for turbo mode related
routines.
One question about the list of p-states in PowerTOP. From the screenshot
you posted, it seems like ptop is listing every turbo mode frequency
along the way. Shouldn't we only display the highest (or last highest
freq) for turbo mode? I'm worried that the list will get too big and
never shrink.
On a wider note, is "turbo mode" the official name for this feature or
is it Intel Dynamic Acceleration (IDA)? I agree that "turbo" is simpler
and easier to use, but I wonder which one would be more appropriate.
Thanks,
Rafael
