Thanks for reviewing this, Aubrey. Comments inline.
Li, Aubrey wrote:
> Rafael Vanoni <rafael.vanoni at sun.com> wrote:
>
>> A good amount of code was rewritten for this patch, here's a
>> summary of the
>> changes to each file. I've posted a webrev @
>> http://cr.opensolaris.org/~rafaelv/ptop-display/
>>
>> battery.c: simple renaming of the routine to follow the rest
>> of the coding
>> style
>
> Looks good.
>
>> cpufreq.c: also renaming routines to follow the overall coding style.
>> pt_cpufreq_check_pm() was extracted from pt_cpufreq_suggest()
>> so that both the
>> routines that suggests turning CPUPM on and the routine that
>> does the change
>> will check if it isn't already activated. This was done to
>> avoid possible
>> duplicated keywords in power.etc. pt_cpufreq_close() closes
>> the DTrace program
>> to freeze this subwindow - for 7953.
>
> Line61 (the line number is after the patch applied)
> Before libpower is available, we have to maintain this method.
Yes, I'm not removing it.
> 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 ?
> Line567:
> We probably don't want to change
> " Enable p-state"
> to
> "Enable PM",
>
> Deep c-state is one of the next suggestion we need to add, which is also
> as one part of PM.
Ok.
>
>> cpuidle.c: pt_cpuidle_close() also for 7953.
>>
>
> Looks good to me.
>
>> display.c: moving #defines from powertop.h here, as these are
>> locally used.
>
> Good point.
>
>> Created a new structure "sb_slot_t" (sb == status bar) to hold
>> each status bar
>> entry. The status bar is now dynamically allocated, anchored
>> at *status_bar,
>> entries are automatically added or removed by calling
>> pt_display_mod_status_bar(), only one per message is allowed.
>>
>> zap_windows() is gone, we don't need to destroy and recreate
>> all subwindows at
>> every refresh. pt_display_init_curses() (also renamed to
>> follow coding style)
>> now checks the size of the terminal window to make sure
>> there's at least room
>> for the states window.
>
> 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.
> Line150:
> why is it necessary here?
> 1) since we are setting up the whole screen for output, do we need to
> erase the whole screen
wnoutrefresh() doesn't erase the screen, it just updated it. But you're
right, we don't need it here.
> 2) refresh status_bar_window is already called in the pt_display_status_bar,
> which is called by pt_display_mod_status_bar
>
> Line173~184:
> Hmm..., it's so complicated to process the window size. I don't think it's
> necessary. event report is an important part of powertop. In this case, I
> don't see
> any point we run powertop without it. Probably just one threshold is enough -
> does
> the window size large enough to display or not.
Well, showing the state report is better than nothing, right? As long as
we tell the user to resize his window if he wants to get all of the data.
> Line329:
> To support my opinion above, I'd like to separate data structure and screen
> process.
> So, remove this line from this routine.
Ok, I'll add calls to update() for every call to mod() separately.
> Line 333:
> Since you rename "show_cstates" to "pt_display_cstates", I want to rename
> further.
> This routine displays pstates as well, how about "pt_display_states"?
Sounds good.
>> events.c: another renaming.
>
> Looks good to me.
>
>> suggestions.c : changing how the display is refreshed and adding the
>> functionality to allow the user to freeze each subwindow (which adds
>> a status bar slot every time a window is freezed) called for the
>> re-write of the status
>> bar and the way suggestions are displayed. In addition to
>> that, I took the
>> liberty to rewrite most of suggestions.c for the following reasons:
>> (a) suggestions were being completely removed and added at every
>> loop iteration, unnecessarily. (b) the routine that selected a
>> suggestion was using a
>> random based hash
>> against the weight to chose one, which I think could be improved.
>> IMHO, suggestions should be ordered by weight and presented to the
>> user in that
>> ordering, but still allowing all existing suggestions to be displayed.
>>
>> The new routine to add suggestions inserts the new one in a
>> liked list ordered
>> from the 'heaviest' to the 'thinnest' suggestion.
>> pt_sugg_pick() walks that
>> list presenting each suggestion for PT_SUGG_DEF_SLICE
>> intervals, with the idea
>> that each suggestion has a time slice, zero'ed when we've
>> displayed them all.
>> When a suggestion is activated, we remove it from the list.
>
> 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
> 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 ?
>> powertop.c: all of the changes above required a handful of
>> changes in main(),
>> mostly notable the addition of the 'I' (idle), 'F' (frequency)
>> and 'E' (events)
>> handlers for pressed keys to implement 7953.
>
> 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.
> 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. 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.
> 3) Line228, I failed to understand why event feature depends on "curses_init"?
> That means event report will be missed in dump mode, this is not reasonable.
That's a bug. I'm initializing curses_init to one now. It will be zero
only if the terminal is too short to display events, but enough for the
states window.
> 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 ?
> 5) Take "I" as an example,
> if pt_cpuidle_stat_prepare() failed, we can't just "break", because the loop
> will
> collect data as long as FEATURE_CSTATE is set.
> =======================================================
> case 'I':
> 349 if (features & FEATURE_CSTATE) {
> 350 pt_cpuidle_close();
> 351 features &= ~FEATURE_CSTATE;
> 352 } else {
> 353 if (pt_cpuidle_stat_prepare() ==
> 0)
> 354 features |=
> FEATURE_CSTATE;
> 355 else
> 356 break; <=== need more
> work here.
> 357 }
> 358 pt_display_mod_status_bar(_("I - Idle"));
> 359 break;
> ======================================================
If pt_cpuidle_stat_prepare() fails, we won't set FEATURE_CSTATE, so we
won't try to collect this data.
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.
Thanks,
Rafael
