Li, Aubrey wrote: > Rafael.Vanoni wrote: > >> 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) > > Take a look at powertop.c. > pt_turbo_stat_collect will only be called when turbo is supported.
Maybe I should've phrased it better. If pt_turbo_snapshot() fails, t_new is not freed. >> - 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? > > That's a good suggestion, but the current implementation makes the powertop.c > cleaner, since we don't clobber it too much. Otherwise you have to add a > variable > to it. I was suggesting making it a local variable to pt_turbo_stat_collect(). I don't see how that would clobber powertop.c.. am I missing something ? >> usr/src/cmd/powertop/powertop.c >> - I'd suggest clearing g_turbo_supported around lines 80-83, before >> we start initializing everything else. > > okay, will do that. > >> 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. > > I can do the change but why do we need to do that? if you mean the helper > functions like the write_ctrl(), :) That one was there before turbo ;) It's a nit, so feel free to ignore it.. But, for instance, I'm suggesting changing update_turbo_info -> turbo_mode_update_info turbo_kstat_update -> turbo_mode_kstat_update record_turbo_info -> turbo_mode_record_info ... >> 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. > > hehehe, that's not true. The highest freq is tagged by the label(turbo). > my Nehalem machine has 12 P-states. Ok. >> 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 > > "turbo" is well known to us, are you suggesting to rename it to "ida"? I'm just wondering which name will be around longer, and suggesting that we use that. > Thanks for your comments, Rafael! My pleasure ;) Rafael
