2019-10-23 03:29:04

by Lubashev, Igor

Subject: [PATCH 0/3] perf: Allow running without stdin

This series allows perf to run in batch mode with stdin closed.
This is arguably a bug fix for code that runs with --stdio option and does not
check for EOF return code from getc().

This series makes the following work as expected:

$ perf top --stdio < /dev/null
$ perf kvm top --stdio < /dev/null

Patches:
01: perf top: Allow running without stdin
Make "perf top --stdio" handle EOF from stdin correctly.
There is one getc() that does not handle EOF explicitly, since its return
value is checked against a list of known characters, and the main loop in
display_thread() will handle the EOF on the next iteration.

02: perf kvm: Allow running without stdin
Make "perf kvm --stdio" handle EOF from stdin correctly.

03: perf kvm: Use evlist layer api when possible
This is a simple fix for a needless layering violation.

Igor Lubashev (3):
perf top: Allow running without stdin
perf kvm: Allow running without stdin
perf kvm: Use evlist layer api when possible

tools/perf/builtin-kvm.c | 35 +++++++++++++++++++++--------------
tools/perf/builtin-top.c | 10 ++++++++--
2 files changed, 29 insertions(+), 16 deletions(-)

--
2.7.4

2019-10-23 03:29:23

by Lubashev, Igor

Subject: [PATCH 3/3] perf kvm: Use evlist layer api when possible

No need for layer violations when a proper evlist api is available.

Signed-off-by: Igor Lubashev <[email protected]>
---
tools/perf/builtin-kvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 5217aa3596c7..340927c2b243 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -1005,7 +1005,7 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
}

if (!rc && !done)
- err = fdarray__poll(fda, 100);
+ err = evlist__poll(kvm->evlist, 100);
}

evlist__disable(kvm->evlist);
--
2.7.4

2019-10-23 05:31:44

by Lubashev, Igor

Subject: [PATCH 2/3] perf kvm: Allow running without stdin

Allow perf kvm --stdio to run without access to stdin.
This lets perf kvm to run in a batch mode until interrupted.

The following now works as expected:

$ perf kvm top --stdio < /dev/null

Signed-off-by: Igor Lubashev <[email protected]>
---
tools/perf/builtin-kvm.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 858da896b518..5217aa3596c7 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -930,18 +930,20 @@ static int fd_set_nonblock(int fd)

static int perf_kvm__handle_stdin(void)
{
- int c;
-
- c = getc(stdin);
- if (c == 'q')
+ switch (getc(stdin)) {
+ case 'q':
+ done = 1;
return 1;
-
- return 0;
+ case EOF:
+ return 0;
+ default:
+ return 1;
+ }
}

static int kvm_events_live_report(struct perf_kvm_stat *kvm)
{
- int nr_stdin, ret, err = -EINVAL;
+ int nr_stdin = -1, ret, err = -EINVAL;
struct termios save;

/* live flag must be set first */
@@ -972,13 +974,16 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
if (evlist__add_pollfd(kvm->evlist, kvm->timerfd) < 0)
goto out;

- nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
- if (nr_stdin < 0)
- goto out;
-
if (fd_set_nonblock(fileno(stdin)) != 0)
goto out;

+ /* add stdin, if it is connected */
+ if (getc(stdin) != EOF) {
+ nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
+ if (nr_stdin < 0)
+ goto out;
+ }
+
/* everything is good - enable the events and process */
evlist__enable(kvm->evlist);

@@ -994,8 +999,10 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
if (err)
goto out;

- if (fda->entries[nr_stdin].revents & POLLIN)
- done = perf_kvm__handle_stdin();
+ if (nr_stdin >= 0 && fda->entries[nr_stdin].revents & POLLIN) {
+ if (!perf_kvm__handle_stdin())
+ fda->entries[nr_stdin].events = 0;
+ }

if (!rc && !done)
err = fdarray__poll(fda, 100);
--
2.7.4

2019-10-23 12:54:59

by Jiri Olsa

Subject: Re: [PATCH 2/3] perf kvm: Allow running without stdin

On Tue, Oct 22, 2019 at 09:54:52PM -0400, Igor Lubashev wrote:
> Allow perf kvm --stdio to run without access to stdin.
> This lets perf kvm to run in a batch mode until interrupted.
>
> The following now works as expected:
>
> $ perf kvm top --stdio < /dev/null
>
> Signed-off-by: Igor Lubashev <[email protected]>
> ---
> tools/perf/builtin-kvm.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 858da896b518..5217aa3596c7 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -930,18 +930,20 @@ static int fd_set_nonblock(int fd)
>
> static int perf_kvm__handle_stdin(void)
> {
> - int c;
> -
> - c = getc(stdin);
> - if (c == 'q')
> + switch (getc(stdin)) {
> + case 'q':
> + done = 1;
> return 1;
> -
> - return 0;
> + case EOF:
> + return 0;
> + default:
> + return 1;
> + }
> }
>
> static int kvm_events_live_report(struct perf_kvm_stat *kvm)
> {
> - int nr_stdin, ret, err = -EINVAL;
> + int nr_stdin = -1, ret, err = -EINVAL;
> struct termios save;
>
> /* live flag must be set first */
> @@ -972,13 +974,16 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
> if (evlist__add_pollfd(kvm->evlist, kvm->timerfd) < 0)
> goto out;
>
> - nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
> - if (nr_stdin < 0)
> - goto out;
> -
> if (fd_set_nonblock(fileno(stdin)) != 0)
> goto out;
>
> + /* add stdin, if it is connected */
> + if (getc(stdin) != EOF) {
> + nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
> + if (nr_stdin < 0)
> + goto out;
> + }
> +
> /* everything is good - enable the events and process */
> evlist__enable(kvm->evlist);
>
> @@ -994,8 +999,10 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
> if (err)
> goto out;
>
> - if (fda->entries[nr_stdin].revents & POLLIN)
> - done = perf_kvm__handle_stdin();
> + if (nr_stdin >= 0 && fda->entries[nr_stdin].revents & POLLIN) {
> + if (!perf_kvm__handle_stdin())

can this return 0 ? if stdin is EOF then nr_stdin stays -1

> + fda->entries[nr_stdin].events = 0;

why do you need to set events to 0 in here?

thanks,
jirka

2019-10-23 21:08:34

by Arnaldo Carvalho de Melo

Subject: Re: [PATCH 3/3] perf kvm: Use evlist layer api when possible

Em Tue, Oct 22, 2019 at 09:54:53PM -0400, Igor Lubashev escreveu:
> No need for layer violations when a proper evlist api is available.

Thanks, applied.

- Arnaldo

> Signed-off-by: Igor Lubashev <[email protected]>
> ---
> tools/perf/builtin-kvm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 5217aa3596c7..340927c2b243 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -1005,7 +1005,7 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
> }
>
> if (!rc && !done)
> - err = fdarray__poll(fda, 100);
> + err = evlist__poll(kvm->evlist, 100);
> }
>
> evlist__disable(kvm->evlist);
> --
> 2.7.4

2019-10-24 05:42:25

by Lubashev, Igor

Subject: RE: [PATCH 2/3] perf kvm: Allow running without stdin

> On Wed, Oct 23, 2019 at 6:43 AM Jiri Olsa <[email protected]> wrote:
>
> On Tue, Oct 22, 2019 at 09:54:52PM -0400, Igor Lubashev wrote:
> > Allow perf kvm --stdio to run without access to stdin.
> > This lets perf kvm to run in a batch mode until interrupted.
> >
> > The following now works as expected:
> >
> > $ perf kvm top --stdio < /dev/null
> >
> > Signed-off-by: Igor Lubashev <[email protected]>
> > ---
> > tools/perf/builtin-kvm.c | 33 ++++++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c index
> > 858da896b518..5217aa3596c7 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> > @@ -930,18 +930,20 @@ static int fd_set_nonblock(int fd)
> >
> > static int perf_kvm__handle_stdin(void) {
> > - int c;
> > -
> > - c = getc(stdin);
> > - if (c == 'q')
> > + switch (getc(stdin)) {
> > + case 'q':
> > + done = 1;
> > return 1;
> > -
> > - return 0;
> > + case EOF:
> > + return 0;
> > + default:
> > + return 1;
> > + }
> > }
> >
> > static int kvm_events_live_report(struct perf_kvm_stat *kvm) {
> > - int nr_stdin, ret, err = -EINVAL;
> > + int nr_stdin = -1, ret, err = -EINVAL;
> > struct termios save;
> >
> > /* live flag must be set first */
> > @@ -972,13 +974,16 @@ static int kvm_events_live_report(struct
> perf_kvm_stat *kvm)
> > if (evlist__add_pollfd(kvm->evlist, kvm->timerfd) < 0)
> > goto out;
> >
> > - nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
> > - if (nr_stdin < 0)
> > - goto out;
> > -
> > if (fd_set_nonblock(fileno(stdin)) != 0)
> > goto out;
> >
> > + /* add stdin, if it is connected */
> > + if (getc(stdin) != EOF) {
> > + nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
> > + if (nr_stdin < 0)
> > + goto out;
> > + }
> > +
> > /* everything is good - enable the events and process */
> > evlist__enable(kvm->evlist);
> >
> > @@ -994,8 +999,10 @@ static int kvm_events_live_report(struct
> perf_kvm_stat *kvm)
> > if (err)
> > goto out;
> >
> > - if (fda->entries[nr_stdin].revents & POLLIN)
> > - done = perf_kvm__handle_stdin();
> > + if (nr_stdin >= 0 && fda->entries[nr_stdin].revents & POLLIN)
> {
> > + if (!perf_kvm__handle_stdin())
>
> can this return 0 ? if stdin is EOF then nr_stdin stays -1
>
> > + fda->entries[nr_stdin].events = 0;
>
> why do you need to set events to 0 in here?

Otherwise poll() would wakeup continually, since an EOF stdin is already ready for POLLIN.

- Igor

> thanks,
> jirka

2019-10-24 23:09:19

by Jiri Olsa

Subject: Re: [PATCH 0/3] perf: Allow running without stdin

On Tue, Oct 22, 2019 at 09:54:50PM -0400, Igor Lubashev wrote:
> This series allows perf to run in batch mode with stdin closed.
> This is arguably a bug fix for code that runs with --stdio option and does not
> check for EOF return code from getc().
>
> This series makes the following work as expected:
>
> $ perf top --stdio < /dev/null
> $ perf kvm top --stdio < /dev/null
>
> Patches:
> 01: perf top: Allow running without stdin
> Make "perf top --stdio" handle EOF from stdin correctly.
> There is one getc() that does not handle EOF explicitly, since its return
> value is checked against a list of known characters, and the main loop in
> display_thread() will handle the EOF on the next iteration.
>
> 02: perf kvm: Allow running without stdin
> Make "perf kvm --stdio" handle EOF from stdin correctly.
>
> 03: perf kvm: Use evlist layer api when possible
> This is a simple fix for a needless layering violation.
>
> Igor Lubashev (3):
> perf top: Allow running without stdin
> perf kvm: Allow running without stdin
> perf kvm: Use evlist layer api when possible

Acked-by: Jiri Olsa <[email protected]>

thanks,
jirka

>
> tools/perf/builtin-kvm.c | 35 +++++++++++++++++++++--------------
> tools/perf/builtin-top.c | 10 ++++++++--
> 2 files changed, 29 insertions(+), 16 deletions(-)
>
> --
> 2.7.4
>

2019-11-08 19:58:09

by Lubashev, Igor

Subject: RE: [PATCH 2/3] perf kvm: Allow running without stdin

Sorry, Jiri! I replied previously to one comment and totally missed another comment above. Mea culpa. Not intentional. Comments inline.

Arnaldo, were you waiting for this comment to take this and the previous patch in the series? (I only see that PATCH 3/3 made it to your tree.)


> On Wed, Oct 23, 2019 at 6:43 AM, Jiri Olsa <[email protected]> wrote:
>
> On Tue, Oct 22, 2019 at 09:54:52PM -0400, Igor Lubashev wrote:
> > Allow perf kvm --stdio to run without access to stdin.
> > This lets perf kvm to run in a batch mode until interrupted.
> >
> > The following now works as expected:
> >
> > $ perf kvm top --stdio < /dev/null
> >
> > Signed-off-by: Igor Lubashev <[email protected]>
> > ---
> > tools/perf/builtin-kvm.c | 33 ++++++++++++++++++++-------------
> > 1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c index
> > 858da896b518..5217aa3596c7 100644
> > --- a/tools/perf/builtin-kvm.c
> > +++ b/tools/perf/builtin-kvm.c
> > @@ -930,18 +930,20 @@ static int fd_set_nonblock(int fd)
> >
> > static int perf_kvm__handle_stdin(void) {
> > - int c;
> > -
> > - c = getc(stdin);
> > - if (c == 'q')
> > + switch (getc(stdin)) {
> > + case 'q':
> > + done = 1;
> > return 1;
> > -
> > - return 0;
> > + case EOF:
> > + return 0;
> > + default:
> > + return 1;
> > + }
> > }
> >
> > static int kvm_events_live_report(struct perf_kvm_stat *kvm) {
> > - int nr_stdin, ret, err = -EINVAL;
> > + int nr_stdin = -1, ret, err = -EINVAL;
> > struct termios save;
> >
> > /* live flag must be set first */
> > @@ -972,13 +974,16 @@ static int kvm_events_live_report(struct
> perf_kvm_stat *kvm)
> > if (evlist__add_pollfd(kvm->evlist, kvm->timerfd) < 0)
> > goto out;
> >
> > - nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
> > - if (nr_stdin < 0)
> > - goto out;
> > -
> > if (fd_set_nonblock(fileno(stdin)) != 0)
> > goto out;
> >
> > + /* add stdin, if it is connected */
> > + if (getc(stdin) != EOF) {
> > + nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
> > + if (nr_stdin < 0)
> > + goto out;
> > + }
> > +
> > /* everything is good - enable the events and process */
> > evlist__enable(kvm->evlist);
> >
> > @@ -994,8 +999,10 @@ static int kvm_events_live_report(struct
> perf_kvm_stat *kvm)
> > if (err)
> > goto out;
> >
> > - if (fda->entries[nr_stdin].revents & POLLIN)
> > - done = perf_kvm__handle_stdin();
> > + if (nr_stdin >= 0 && fda->entries[nr_stdin].revents & POLLIN) {
> > + if (!perf_kvm__handle_stdin())
>
> can this return 0 ? if stdin is EOF then nr_stdin stays -1

The purpose of this check is to catch a case when stdin does not start out closed but gets closed later.
Something like:

sleep 5 | perf kvm ......

Arguably I could had handled both cases in a single uniform way (just this code without the initial check). The effect of this would be one unexpected cycle of output at the very beginning in the common case (stdin is closed from the start). So I wanted to make the common case behave well.


> > + fda->entries[nr_stdin].events = 0;
>
> why do you need to set events to 0 in here?

As I mentioned before, this is to ensure that no more wakeups are going to happen due to stdin getting closed during the perf run. W/o this, stdin fd would always stay "ready for POLLIN", and perf would never sleep. (The cleanest way would be to remove stdin fd from the kvm->evlist, but there is no such interface for evlist. Writing that interface just for this case seemed imprudent, when events=0 would do, and we already access revents above.)

> thanks,
> jirka

Best,

- Igor

2019-11-12 11:21:20

by tip-bot2 for Thomas Gleixner

Subject: [tip: perf/core] perf kvm: Use evlist layer api when possible

The following commit has been merged into the perf/core branch of tip:

Commit-ID: 4bfbcf3ee1cce3c2ea9870287fc34d3391d5a9b0
Gitweb: https://git.kernel.org/tip/4bfbcf3ee1cce3c2ea9870287fc34d3391d5a9b0
Author: Igor Lubashev <[email protected]>
AuthorDate: Tue, 22 Oct 2019 21:54:53 -04:00
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitterDate: Wed, 06 Nov 2019 15:43:05 -03:00

perf kvm: Use evlist layer api when possible

No need for layer violations when a proper evlist api is available.

Signed-off-by: Igor Lubashev <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-kvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 858da89..577af4f 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -998,7 +998,7 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
done = perf_kvm__handle_stdin();

if (!rc && !done)
- err = fdarray__poll(fda, 100);
+ err = evlist__poll(kvm->evlist, 100);
}

evlist__disable(kvm->evlist);