Li, Aubrey 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. > > 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.
How about making a static pointer inside turbo.c and allocating it once during pt_turbo_init()? I'm trying to avoid alloc'ing and free'ing it every time we take a snapshot. Thanks, Rafael >>>> 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 > _______________________________________________ > tesla-dev mailing list > tesla-dev at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/tesla-dev
