Re: [PATCH 09/24] perf daemon: Add signalfd support

[Jiri Olsa <jolsa@redhat.com>]

On Wed, Feb 03, 2021 at 06:24:09PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> > @@ -351,6 +355,103 @@ static int session__run(struct session *session, struct daemon *daemon)
> >  	return -1;
> >  }
> >  
> > +static pid_t handle_signalfd(struct daemon *daemon)
> > +{
> > +	struct signalfd_siginfo si;
> > +	struct session *session;
> > +	ssize_t err;
> > +	int status;
> > +	pid_t pid;
> > +
> > +	err = read(daemon->signal_fd, &si, sizeof(struct signalfd_siginfo));
> 
> 
> I saw these and recalled we have a readn() function, shouldn't we be
> usint it in this series?

right, but the read call in here needs to succeed with the data
size otherwise it's an error

but perhaps we could use it later for the control descriptor
read/write, I'll check

> 
> Its even in tools/lib/perf/lib.c
> 
> > +	if (err != sizeof(struct signalfd_siginfo))
> > +		return -1;
> > +
> > +	list_for_each_entry(session, &daemon->sessions, list) {
> > +
> > +		if (session->pid != (int) si.ssi_pid)
> > +			continue;
> > +
> > +		pid = waitpid(session->pid, &status, 0);
> > +		if (pid == session->pid) {
> > +			if (WIFEXITED(status)) {
> > +				pr_info("session '%s' exited, status=%d\n",
> > +					session->name, WEXITSTATUS(status));
> > +			} else if (WIFSIGNALED(status)) {
> > +				pr_info("session '%s' killed (signal %d)\n",
> > +					session->name, WTERMSIG(status));
> > +			} else if (WIFSTOPPED(status)) {
> > +				pr_info("session '%s' stopped (signal %d)\n",
> > +					session->name, WSTOPSIG(status));
> > +			} else {
> > +				pr_info("session '%s' Unexpected status (0x%x)\n",
> > +					session->name, status);
> > +			}
> > +		}
> > +
> > +		session->state = SESSION_STATE__KILL;
> > +		session->pid = -1;
> > +		return pid;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int session__wait(struct session *session, struct daemon *daemon, int secs)
> > +{
> > +	struct pollfd pollfd = {
> > +		.fd	= daemon->signal_fd,
> > +		.events	= POLLIN,
> > +	};
> > +	pid_t wpid = 0, pid = session->pid;
> > +	time_t start;
> > +
> > +	start = time(NULL);
> 
> 
> 	time_t start = time(NULL);
> > +
> > +	do {
> > +		if (poll(&pollfd, 1, 1000))
> > +			wpid = handle_signalfd(daemon);
> 
> Shouldn't we check for -1 and handle that differently?

ah right, check for error, will add

> 
> > +
> > +		if (start + secs < time(NULL))
> > +			return -1;
> > +	} while (wpid != pid);
> > +
> > +	return 0;
> > +}
> > +
> > +static bool daemon__has_alive_session(struct daemon *daemon)
> > +{
> > +	struct session *session;
> > +
> > +	list_for_each_entry(session, &daemon->sessions, list) {
> > +		if (session->pid != -1)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +static int daemon__wait(struct daemon *daemon, int secs)
> > +{
> > +	struct pollfd pollfd = {
> > +		.fd	= daemon->signal_fd,
> > +		.events	= POLLIN,
> > +	};
> > +	time_t start;
> > +
> > +	start = time(NULL);
> > +
> > +	do {
> > +		if (poll(&pollfd, 1, 1000))
> > +			handle_signalfd(daemon);
> 
> ditto

ok

thanks,
jirka