* Re: [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified
@ 2014-08-20 12:28 Sergey Senozhatsky
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2014-08-20 12:28 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]
On (08/05/14 22:31), Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
> ---
> src/main.cpp | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
I think it makes sense to extend ui_notify_user() so it can cover
both cases: initialised ncurses -- calling mvprintw(), and
uninitialised ncurses -- printf (to stdout).
this will change a bit 'powertop --auto-tune' behaviour, since by
default user will now see a 'list' of commands executed by powertop,
but it's really easy to silent powertop via stdout redirection.
what do you think?
-ss
> diff --git a/src/main.cpp b/src/main.cpp
> index 3b67ef0..4bfd038 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -460,7 +460,8 @@ int main(int argc, char **argv)
> end_pci_access();
> exit(0);
> }
> - init_display();
> + if (!auto_tune)
> + init_display();
> initialize_tuning();
> /* first one is short to not let the user wait too long */
> one_measurement(1, NULL);
> @@ -473,11 +474,13 @@ int main(int argc, char **argv)
> }
>
> while (!leave_powertop) {
> - show_cur_tab();
> + if (!auto_tune)
> + show_cur_tab();
> one_measurement(time_out, NULL);
> learn_parameters(15, 0);
> }
> - endwin();
> + if (!auto_tune)
> + endwin();
> printf("%s\n", _("Leaving PowerTOP"));
>
> end_process_data();
> --
> 2.0.4
>
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified
@ 2014-08-21 11:54 Sergey Senozhatsky
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2014-08-21 11:54 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]
On (08/20/14 23:56), Sami Kerola wrote:
> Hello Sergey and others,
>
> I had a bit of spare time so I started to look what exactly is calling
> mvprintw() via ui_notify_user(). My initial reaction was that I must
> have made some error in search, because I could not find such call. So I
> added abort() to the code...
>
> index b482296..55c9603 100644
> --- a/src/lib.cpp
> +++ b/src/lib.cpp
> @@ -546,5 +546,6 @@ void ui_notify_user(const char *frmt, ...)
> vsnprintf(notify, UI_NOTIFY_BUFF_SZ - 1, frmt, list);
> va_end(list);
> mvprintw(1, 0, notify);
> + abort();
> attroff(COLOR_PAIR(1));
> }
>
> ...and found the --auto-tune does not entering to that function. When
> running in normal mode the call path is from main() -> one_measurement()
> -> do_sleep() -> cursor_enter() -> tuning_window::cursor_enter ->
> ui_notify_user() -> mvprintw().
>
oh, yes. my bad, I didn't double check.
auto-tune calls all_tunables[i]->toggle() directly.
ok. then everything should be fine.
-ss
> I also realized the --auto-tune is setting leave_powertop = 1, which
> means the patch 0009 can be simplified. Here is better version of the
> change that avoids running unnecessary if().
>
> --->8----
> From: Sami Kerola <kerolasa(a)iki.fi>
> Date: Wed, 20 Aug 2014 23:31:52 +0300
> Subject: [PATCH 09/14] do not use ncurses when --auto-tune is specified
>
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
> ---
> src/main.cpp | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/main.cpp b/src/main.cpp
> index 3b67ef0..bd0fc88 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -460,7 +460,8 @@ int main(int argc, char **argv)
> end_pci_access();
> exit(0);
> }
> - init_display();
> + if (!auto_tune)
> + init_display();
> initialize_tuning();
> /* first one is short to not let the user wait too long */
> one_measurement(1, NULL);
> @@ -477,7 +478,8 @@ int main(int argc, char **argv)
> one_measurement(time_out, NULL);
> learn_parameters(15, 0);
> }
> - endwin();
> + if (!auto_tune)
> + endwin();
> printf("%s\n", _("Leaving PowerTOP"));
>
> end_process_data();
> --
> 2.1.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified
@ 2014-08-20 20:56 Sami Kerola
0 siblings, 0 replies; 6+ messages in thread
From: Sami Kerola @ 2014-08-20 20:56 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 3043 bytes --]
On 20 August 2014 15:57, Sami Kerola <kerolasa(a)iki.fi> wrote:
> On 20 August 2014 15:28, Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com> wrote:
>> On (08/05/14 22:31), Sami Kerola wrote:
>>> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
>>> ---
>>> src/main.cpp | 9 ++++++---
>>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>
>> I think it makes sense to extend ui_notify_user() so it can cover
>> both cases: initialised ncurses -- calling mvprintw(), and
>> uninitialised ncurses -- printf (to stdout).
>>
>> this will change a bit 'powertop --auto-tune' behaviour, since by
>> default user will now see a 'list' of commands executed by powertop,
>> but it's really easy to silent powertop via stdout redirection.
>>
>>
>> what do you think?
>
> I think this patch set will need to get ready eventually. Would it be
> possible that the upstream maintainer will drop the patch 0007 aka 'do
> not use ncurses' and merge the rest (assuming they are not unmentioned
> way borken).
Hello Sergey and others,
I had a bit of spare time so I started to look what exactly is calling
mvprintw() via ui_notify_user(). My initial reaction was that I must
have made some error in search, because I could not find such call. So I
added abort() to the code...
index b482296..55c9603 100644
--- a/src/lib.cpp
+++ b/src/lib.cpp
@@ -546,5 +546,6 @@ void ui_notify_user(const char *frmt, ...)
vsnprintf(notify, UI_NOTIFY_BUFF_SZ - 1, frmt, list);
va_end(list);
mvprintw(1, 0, notify);
+ abort();
attroff(COLOR_PAIR(1));
}
...and found the --auto-tune does not entering to that function. When
running in normal mode the call path is from main() -> one_measurement()
-> do_sleep() -> cursor_enter() -> tuning_window::cursor_enter ->
ui_notify_user() -> mvprintw().
I also realized the --auto-tune is setting leave_powertop = 1, which
means the patch 0009 can be simplified. Here is better version of the
change that avoids running unnecessary if().
--->8----
From: Sami Kerola <kerolasa(a)iki.fi>
Date: Wed, 20 Aug 2014 23:31:52 +0300
Subject: [PATCH 09/14] do not use ncurses when --auto-tune is specified
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky(a)gmail.com>
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/main.cpp | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index 3b67ef0..bd0fc88 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -460,7 +460,8 @@ int main(int argc, char **argv)
end_pci_access();
exit(0);
}
- init_display();
+ if (!auto_tune)
+ init_display();
initialize_tuning();
/* first one is short to not let the user wait too long */
one_measurement(1, NULL);
@@ -477,7 +478,8 @@ int main(int argc, char **argv)
one_measurement(time_out, NULL);
learn_parameters(15, 0);
}
- endwin();
+ if (!auto_tune)
+ endwin();
printf("%s\n", _("Leaving PowerTOP"));
end_process_data();
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified
@ 2014-08-20 12:57 Sami Kerola
0 siblings, 0 replies; 6+ messages in thread
From: Sami Kerola @ 2014-08-20 12:57 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]
On 20 August 2014 15:28, Sergey Senozhatsky
<sergey.senozhatsky(a)gmail.com> wrote:
> On (08/05/14 22:31), Sami Kerola wrote:
>> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
>> ---
>> src/main.cpp | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>
> I think it makes sense to extend ui_notify_user() so it can cover
> both cases: initialised ncurses -- calling mvprintw(), and
> uninitialised ncurses -- printf (to stdout).
>
> this will change a bit 'powertop --auto-tune' behaviour, since by
> default user will now see a 'list' of commands executed by powertop,
> but it's really easy to silent powertop via stdout redirection.
>
>
> what do you think?
Hi Sergey,
I think this patch set will need to get ready eventually. Would it be
possible that the upstream maintainer will drop the patch 0007 aka 'do
not use ncurses' and merge the rest (assuming they are not unmentioned
way borken).
Since my primary goal when I started to do anything with powertop was
to get rid of --auto-tune blinking I will craft another change that
will do exactly that. But as mentioned, there is error reporting that
must not break as side effect. Because a signal to do printing
different way is command line option driven it is probably best to add
a control structure that will allow to do that, and allow easy
extensions in future when similar needs appear.
What comes to stdout redirection, you are likely right. Making ncurses
not to blink screen may require poking code a little bit here and
there. To me that is a another good reason to separate the change set.
--
Sami Kerola
http://www.iki.fi/kerolasa/
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified
@ 2014-08-19 11:41 Sergey Senozhatsky
0 siblings, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2014-08-19 11:41 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]
On (08/05/14 22:31), Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
> ---
> src/main.cpp | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/src/main.cpp b/src/main.cpp
> index 3b67ef0..4bfd038 100644
> --- a/src/main.cpp
> +++ b/src/main.cpp
> @@ -460,7 +460,8 @@ int main(int argc, char **argv)
> end_pci_access();
> exit(0);
> }
> - init_display();
> + if (!auto_tune)
> + init_display();
tuning use ui_notify_user(), which requires ncurses. yes, there
is only one place where we notify user via ui_notify_user(), but,
in general, we can (and should) use this function for error
reporting back to user as well, since fprintf to stderr is less
correct/nice way to yell about something important/critical.
-ss
> initialize_tuning();
> /* first one is short to not let the user wait too long */
> one_measurement(1, NULL);
> @@ -473,11 +474,13 @@ int main(int argc, char **argv)
> }
>
> while (!leave_powertop) {
> - show_cur_tab();
> + if (!auto_tune)
> + show_cur_tab();
> one_measurement(time_out, NULL);
> learn_parameters(15, 0);
> }
> - endwin();
> + if (!auto_tune)
> + endwin();
> printf("%s\n", _("Leaving PowerTOP"));
>
> end_process_data();
> --
> 2.0.4
>
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified
@ 2014-08-05 21:31 Sami Kerola
0 siblings, 0 replies; 6+ messages in thread
From: Sami Kerola @ 2014-08-05 21:31 UTC (permalink / raw)
To: powertop
[-- Attachment #1: Type: text/plain, Size: 865 bytes --]
Signed-off-by: Sami Kerola <kerolasa(a)iki.fi>
---
src/main.cpp | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/main.cpp b/src/main.cpp
index 3b67ef0..4bfd038 100644
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -460,7 +460,8 @@ int main(int argc, char **argv)
end_pci_access();
exit(0);
}
- init_display();
+ if (!auto_tune)
+ init_display();
initialize_tuning();
/* first one is short to not let the user wait too long */
one_measurement(1, NULL);
@@ -473,11 +474,13 @@ int main(int argc, char **argv)
}
while (!leave_powertop) {
- show_cur_tab();
+ if (!auto_tune)
+ show_cur_tab();
one_measurement(time_out, NULL);
learn_parameters(15, 0);
}
- endwin();
+ if (!auto_tune)
+ endwin();
printf("%s\n", _("Leaving PowerTOP"));
end_process_data();
--
2.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-21 11:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-20 12:28 [Powertop] [PATCH 09/14] do not use ncurses when --auto-tune is specified Sergey Senozhatsky
-- strict thread matches above, loose matches on Subject: below --
2014-08-21 11:54 Sergey Senozhatsky
2014-08-20 20:56 Sami Kerola
2014-08-20 12:57 Sami Kerola
2014-08-19 11:41 Sergey Senozhatsky
2014-08-05 21:31 Sami Kerola
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.