Hey Aubrey, have you had a chance to review the last batch of changes ? Thanks, Rafael
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
