Mark Haywood wrote: > 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. >
Of course things have changed over time and it does not appear that the rule any longer applies. ;-) > > >>>> 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 >> >> > > _______________________________________________ > tesla-dev mailing list > tesla-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/tesla-dev >
