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