Rafael Vanoni wrote: > 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 ... > >
The precedence I set with speedstep.c was that externally accessible functions were prefixed. I didn't really care if static routines were prefixed. >>> 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 > _______________________________________________ > tesla-dev mailing list > tesla-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/tesla-dev >
