Li, Aubrey wrote: > 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.
We don't need to file a case for this. It's a feature of the app, not a new interface. cheers, Rafael > 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 >
