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

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

[Igor Lubashev]

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

[PATCH 1/3] perf top: Allow running without stdin

[Igor Lubashev]

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

The following now works as expected:

  $ perf top < /dev/null

Signed-off-by: Igor Lubashev <ilubashe@akamai.com>
---
 tools/perf/builtin-top.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index d96f24c8770d..fbc0dc135b8a 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -665,6 +665,7 @@ static void display_setup_sig(void)
 static void *display_thread(void *arg)
 {
 	struct pollfd stdin_poll = { .fd = 0, .events = POLLIN };
+	nfds_t nfds = 1;
 	struct termios save;
 	struct perf_top *top = arg;
 	int delay_msecs, c;
@@ -684,7 +685,8 @@ static void *display_thread(void *arg)
 	delay_msecs = top->delay_secs * MSEC_PER_SEC;
 	set_term_quiet_input(&save);
 	/* trash return*/
-	getc(stdin);
+	if (getc(stdin) == EOF)
+		nfds = 0;
 
 	while (!done) {
 		perf_top__print_sym_table(top);
@@ -692,7 +694,7 @@ static void *display_thread(void *arg)
 		 * Either timeout expired or we got an EINTR due to SIGWINCH,
 		 * refresh screen in both cases.
 		 */
-		switch (poll(&stdin_poll, 1, delay_msecs)) {
+		switch (poll(&stdin_poll, nfds, delay_msecs)) {
 		case 0:
 			continue;
 		case -1:
@@ -701,6 +703,10 @@ static void *display_thread(void *arg)
 			__fallthrough;
 		default:
 			c = getc(stdin);
+			if (c == EOF) {
+				nfds = 0;
+				continue;
+			}
 			tcsetattr(0, TCSAFLUSH, &save);
 
 			if (perf_top__handle_keypress(top, c))
-- 
2.7.4

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

[Igor Lubashev]

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 <ilubashe@akamai.com>
---
 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

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

[Igor Lubashev]

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

Signed-off-by: Igor Lubashev <ilubashe@akamai.com>
---
 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

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

[Jiri Olsa]

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 <ilubashe@akamai.com>
> ---
>  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

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

[Arnaldo Carvalho de Melo]

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 <ilubashe@akamai.com>
> ---
>  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

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

[Lubashev, Igor]

> On Wed, Oct 23, 2019 at 6:43 AM Jiri Olsa <jolsa@redhat.com> 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 <ilubashe@akamai.com>
> > ---
> >  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

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

[Jiri Olsa]

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 <jolsa@kernel.org>

thanks,
jirka

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

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

[Lubashev, Igor]

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 <jolsa@redhat.com> 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 <ilubashe@akamai.com>
> > ---
> >  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

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

[tip-bot2 for Igor Lubashev]

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 <ilubashe@akamai.com>
AuthorDate:    Tue, 22 Oct 2019 21:54:53 -04:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
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 <ilubashe@akamai.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/1571795693-23558-4-git-send-email-ilubashe@akamai.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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);