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. > - 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. > > 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(), :) > > 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. > > 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"? Thanks for your comments, Rafael! -Aubrey
