Rafael.Vanoni wrote: > Li, Aubrey wrote: >> Hi Rafael, >> >> Rafael.Vanoni wrote: >> >>> Hey Aubrey, have you had a chance to review the last batch of >>> changes ? >>> >>> Thanks, >>> Rafael >> >> More concerns besides the code, since this patch introduces a new >> user interface, do we need another PSARC? > > I'm not sure, I have to ask around. > >> Recently, we fixed several critical bugs, I'm thinking if we can >> release ptop1.2 including the critical bug-fixes, and putback into >> ON soon. > > I'm starting the process to put those fixes back into Nevada, but I'd > like to get this one too to save the extra work, and I also have > fixes for 8658 and 2861 on the way. > >> I'd like to separate this patch into two, >> 1) display.c needs some lovin' >> 2) PowerTOP should allow the user to freeze its subwindows >> >> And I think 2) can be based on ptop1.2. >> >> What do you think? > > I started with that, but implementing the freeze options > required a lot > of changes in display.c. The code really is interleaved. > > I'll propose fixes for those two bugs by COB tomorrow, at the latest. > I'd *really* like to get all the open bugs fixed before we bump the > version. What do you think ?
I think that depends on if PSARC is needed. If it's needed, I'll suggest we commit the new feature after ptop1.2 release. Thanks, -Aubrey > > Thanks, > Rafael > >> Thanks, >> -Aubrey >> >>> Rafael Vanoni wrote: >>>> Li, Aubrey wrote: >>>>> Rafael.Vanoni wrote: >>>>> >>>>>>>> cpufreq.c: >>>>>>> We probably need >>>>>>> "echo cpupm enable poll-mode >> /etc/power.conf" since PAD >>>>>>> introduced another mode. >>>>>> Hmm.. but that's a suggestion that would degrade performance >>>>>> for most of >>>>>> the cases, right ? >>>>> No, poll mode doesn't degrade performance. And it's the default >>>>> mode before. >>>> I meant power performance. We'd be suggesting a regression. >>>> >>>>>>> Line 147~148: >>>>>>> Display behavior is changed here, previously we just add the >>>>>>> message to the suggestion array, now we display them here. >>>>>>> That might be why previously the cursor is flicking at the >>>>>>> status window while now it is flicking at the acpi power window. >>>>>>> I prefer the previous behavior. >>>>>> The change in behavior is that now we refresh the status bar only >>>>>> when it is modified, as opposed to at every interval. If you >>>>>> don't like having the cursor at the new position, we can move it >>>>>> aside. However, we shouldn't even see the cursor after calling >>>>>> curs_set(0). There's probably a separate bug here. >>>>> Sound like more work is needed here. >>>> I'll file a separate bug. >>>> >>>>>>>> suggestions.c : >>>>>>> 1) why calloc(1, ***) here? why not malloc? >>>>>> To zero the new data structure since we check for NULL pointers >>>>>> in its members in a few places outside suggestions.c Sounds good >>>>>> to me. >>>>> >>>>> >>>>>>> 2) removing suggestion needs more thoughts here. >>>>>>> suggestion like cpu frequncy is only added at the beginning, >>>>>>> if the user disabled p-state after it's removed, the suggestion >>>>>>> will be lost forever. >>>>>> I thought of that too. But I think it's safe to assume that if >>>>>> the user has the ability to turn CPUPM on/off through PowerTOP, >>>>>> he would likely be the one to change power.conf from some other >>>>>> interface. I'm not a big >>>>>> fan of the idea of reading power.conf all the time just to cover >>>>>> this scenario. Do you have a suggestion to cover this ? >>>>> I think code should be designed to cover every case, since what we >>>>> assume is not right, :) So, we need to keep the suggestion and >>>>> check every loop. what do you think? >>>> I guess it's okay.. the current version is already opening the >>>> file at every interval. >>>> >>>>>>>> powertop.c: >>>>>>> 1) moving "Collecting data" string ahead looks unreasonable. >>>>>>> The time dtrace preparing cost is visiable. >>>>>> I moved it to before the curses initialization because once we do >>>>>> init curses, printf'inf anything to the screen has undefined >>>>>> behavior. Try moving this call to after pt_display_init_curses() >>>>>> and exiting. The following line is half way across the screen. >>>>> I mean it should be after dtrace preparation, you can deal with >>>>> curses initialization. >>>> Ok, I can move it after we prepare the c and p-states reports, but >>>> if there's not enough size in the window, we don't need to prepare >>>> the event report. >>>> >>>>>>> 2) Taking the curses initialization out of the loop seems to be >>>>>>> right, but I saw you just put the pstate prepare before the >>>>>>> initialization, which is wrong. You're assume P-state number > >>>>>>> C-state number, which is not correct on many platforms or >>> many cases. >>>>>> I wouldn't say it's wrong - in practice, all the systems we >>>>>> support deep c-states have more p-states. >>>>> PowerTOP can run on the old system, which has two cstates (C0 and >>>>> C1) and only one P-state(speedstep is not supported). >>>>> So, in practice, the assume is wrong. >>>>> >>>>> >>>>>> We do a max((g_max_cstate+1), g_npstates); >>>>>> >>>>>> to decide how tall the state window should be, which is biased >>>>>> since we can't trust supported_max_cstates. >>>>> g_max_cstate is assigned in pt_cpuidle_stat_collect(). so when >>>>> call from curses initialization, I believe it's ZERO. >>>> Let's initialize g_max_cstate with 1 to cover the case of not >>>> supporting frequency scaling. That way the max() will do the right >>>> thing. What do you think ? >>>> >>>>>>> 4)"I", "F", "E" feature looks pretty cool, do you want to make >>>>>>> them visible at the beginning? Otherwise it's hard to the user >>>>>>> to know this funcationlity. >>>>>> How about a suggestion ? >>>>> I mean, when powertop start up, the user can see "I", "F", "E" on >>>>> the status window. >>>> But that way, if the user presses one of those keys and freezes a >>>> subwindow, what would we display in the status bar? I've added a >>>> text-only suggestion. Let me know what you think. >>>> >>>>>> If pt_cpuidle_stat_prepare() fails, we won't set FEATURE_CSTATE, >>>>>> so we won't try to collect this data. Looks good. >>>>> >>>>>> I'll update the webrev and the bug report with these changes >>>>>> within the hour. I'm also adding a handler to the window >>>>>> resizing signal. >>>>>> >>>> Just updated the webrev. >>>> >>>> http://cr.opensolaris.org/~rafaelv/ptop-display/ >>>> >>>> Thanks, >>>> Rafael >>>> >>>> >>>> >>>> _______________________________________________ >>>> tesla-dev mailing list >>>> tesla-dev at opensolaris.org >>>> http://mail.opensolaris.org/mailman/listinfo/tesla-dev
