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 ? 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 >
