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
