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.
Hmm..., I misunderstood. thanks to point it out. > >>> - 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 ? It should be an array. How do you define its size? the size should be the number of cpus. Dynamical size is better here, I think. > >>> 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. > I still prefer turbo here, unless there is an objection with reasons, :) Thanks, -Aubrey
