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.
We probably need
"echo cpupm enable poll-mode >> /etc/power.conf" since PAD introduced
another mode.
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.
>
> 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.
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
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.
Line329:
To support my opinion above, I'd like to separate data structure and screen
process.
So, remove this line from this routine.
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"?
>
> 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?
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.
>
> 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.
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.
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.
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.
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;
======================================================
Thanks,
-Aubrey
