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. >> 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. >>> 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? > >>> 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. > >> 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. > >> 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. > > 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. > Thanks, -Aubrey
