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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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