[PATCH v1 1/2] tools api: Add simple timeout to io read

[PATCH v1 1/2] tools api: Add simple timeout to io read

[Ian Rogers]

In situations like reading from a pipe it can be useful to have a
timeout so that the caller doesn't block indefinitely. Implement a
simple one based on poll.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/api/io.h | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
index d5e8cf0dada0..9fc429d2852d 100644
--- a/tools/lib/api/io.h
+++ b/tools/lib/api/io.h
@@ -8,6 +8,7 @@
 #define __API_IO__
 
 #include <errno.h>
+#include <poll.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
@@ -23,6 +24,8 @@ struct io {
 	char *end;
 	/* Currently accessed data pointer. */
 	char *data;
+	/* Read timeout, 0 implies no timeout. */
+	int timeout_ms;
 	/* Set true on when the end of file on read error. */
 	bool eof;
 };
@@ -35,6 +38,7 @@ static inline void io__init(struct io *io, int fd,
 	io->buf = buf;
 	io->end = buf;
 	io->data = buf;
+	io->timeout_ms = 0;
 	io->eof = false;
 }
 
@@ -47,7 +51,29 @@ static inline int io__get_char(struct io *io)
 		return -1;
 
 	if (ptr == io->end) {
-		ssize_t n = read(io->fd, io->buf, io->buf_len);
+		ssize_t n;
+
+		if (io->timeout_ms != 0) {
+			struct pollfd pfds[] = {
+				{
+					.fd = io->fd,
+					.events = POLLIN,
+				},
+			};
+
+			n = poll(pfds, 1, io->timeout_ms);
+			if (n == 0)
+				errno = ETIMEDOUT;
+			if (n > 0 && !(pfds[0].revents & POLLIN)) {
+				errno = EIO;
+				n = -1;
+			}
+			if (n <= 0) {
+				io->eof = true;
+				return -1;
+			}
+		}
+		n = read(io->fd, io->buf, io->buf_len);
 
 		if (n <= 0) {
 			io->eof = true;
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH v1 2/2] perf srcline: Add a timeout to reading from addr2line

[Ian Rogers]

addr2line may fail to send expected values causing perf to wait
indefinitely. Add a 1 second timeout (twice the timeout for reading
from /proc/pid/maps) so that such reads don't cause perf to appear to
lock up.

There are already checks that the file for addr2line contains a debug
section but this isn't always sufficient. The problem was observed
when a valid elf file would set the configuration for binutils
addr2line, then a later read of vmlinux with ELF debug sections would
cause a failing write/read which would block indefinitely.

As a service to future readers, if the io hits eof or an error,
cleanup the addr2line process.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/config.c  |  7 +++++--
 tools/perf/util/srcline.c | 11 ++++++++---
 tools/perf/util/srcline.h |  1 +
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index f340dc73db6d..46f144c46827 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -19,6 +19,7 @@
 #include "util/llvm-utils.h"   /* perf_llvm_config */
 #include "util/stat.h"  /* perf_stat__set_big_num */
 #include "util/evsel.h"  /* evsel__hw_names, evsel__use_bpf_counters */
+#include "util/srcline.h"  /* addr2line_timeout_ms */
 #include "build-id.h"
 #include "debug.h"
 #include "config.h"
@@ -434,12 +435,14 @@ static int perf_buildid_config(const char *var, const char *value)
 	return 0;
 }
 
-static int perf_default_core_config(const char *var __maybe_unused,
-				    const char *value __maybe_unused)
+static int perf_default_core_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "core.proc-map-timeout"))
 		proc_map_timeout = strtoul(value, NULL, 10);
 
+	if (!strcmp(var, "core.addr2line-timeout"))
+		addr2line_timeout_ms = strtoul(value, NULL, 10);
+
 	/* Add other config variables here. */
 	return 0;
 }
diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
index cfca03abd6f8..e06a345c5d19 100644
--- a/tools/perf/util/srcline.c
+++ b/tools/perf/util/srcline.c
@@ -21,6 +21,8 @@
 #include "symbol.h"
 #include "subcmd/run-command.h"
 
+/* If addr2line doesn't return data for 1 second then timeout. */
+int addr2line_timeout_ms = 1 * 1000;
 bool srcline_full_filename;
 
 static const char *dso__name(struct dso *dso)
@@ -512,7 +514,6 @@ static int read_addr2line_record(struct io *io,
 		zfree(&line);
 		return 0;
 	}
-
 	if (function != NULL)
 		*function = strdup(strim(line));
 
@@ -574,7 +575,7 @@ static int addr2line(const char *dso_name, u64 addr,
 	int len;
 	char buf[128];
 	ssize_t written;
-	struct io io;
+	struct io io = { .eof = false };
 	enum a2l_style a2l_style;
 
 	if (!a2l) {
@@ -616,7 +617,7 @@ static int addr2line(const char *dso_name, u64 addr,
 		goto out;
 	}
 	io__init(&io, a2l->out, buf, sizeof(buf));
-
+	io.timeout_ms = addr2line_timeout_ms;
 	switch (read_addr2line_record(&io, a2l_style,
 				      &record_function, &record_filename, &record_line_nr)) {
 	case -1:
@@ -686,6 +687,10 @@ static int addr2line(const char *dso_name, u64 addr,
 out:
 	free(record_function);
 	free(record_filename);
+	if (io.eof) {
+		dso->a2l = NULL;
+		addr2line_subprocess_cleanup(a2l);
+	}
 	return ret;
 }
 
diff --git a/tools/perf/util/srcline.h b/tools/perf/util/srcline.h
index b11a0aaaa676..35a5ff3e78f5 100644
--- a/tools/perf/util/srcline.h
+++ b/tools/perf/util/srcline.h
@@ -9,6 +9,7 @@
 struct dso;
 struct symbol;
 
+extern int addr2line_timeout_ms;
 extern bool srcline_full_filename;
 char *get_srcline(struct dso *dso, u64 addr, struct symbol *sym,
 		  bool show_sym, bool show_addr, u64 ip);
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH v1 1/2] tools api: Add simple timeout to io read

[Ian Rogers]

On Wed, Jun 7, 2023 at 11:20 PM Ian Rogers <irogers@google.com> wrote:
>
> In situations like reading from a pipe it can be useful to have a
> timeout so that the caller doesn't block indefinitely. Implement a
> simple one based on poll.
>
> Signed-off-by: Ian Rogers <irogers@google.com>

There is overlap in what these patches aim to fix with the 2 submitted
patches making addr2line more robust:
https://lore.kernel.org/all/20230613034817.1356114-2-irogers@google.com/

I think it could be pragmatic to take both of them. Be robust but
timeout if addr2line doesn't respond for 1s. What do you think?

Thanks,
Ian

> ---
>  tools/lib/api/io.h | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> index d5e8cf0dada0..9fc429d2852d 100644
> --- a/tools/lib/api/io.h
> +++ b/tools/lib/api/io.h
> @@ -8,6 +8,7 @@
>  #define __API_IO__
>
>  #include <errno.h>
> +#include <poll.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> @@ -23,6 +24,8 @@ struct io {
>         char *end;
>         /* Currently accessed data pointer. */
>         char *data;
> +       /* Read timeout, 0 implies no timeout. */
> +       int timeout_ms;
>         /* Set true on when the end of file on read error. */
>         bool eof;
>  };
> @@ -35,6 +38,7 @@ static inline void io__init(struct io *io, int fd,
>         io->buf = buf;
>         io->end = buf;
>         io->data = buf;
> +       io->timeout_ms = 0;
>         io->eof = false;
>  }
>
> @@ -47,7 +51,29 @@ static inline int io__get_char(struct io *io)
>                 return -1;
>
>         if (ptr == io->end) {
> -               ssize_t n = read(io->fd, io->buf, io->buf_len);
> +               ssize_t n;
> +
> +               if (io->timeout_ms != 0) {
> +                       struct pollfd pfds[] = {
> +                               {
> +                                       .fd = io->fd,
> +                                       .events = POLLIN,
> +                               },
> +                       };
> +
> +                       n = poll(pfds, 1, io->timeout_ms);
> +                       if (n == 0)
> +                               errno = ETIMEDOUT;
> +                       if (n > 0 && !(pfds[0].revents & POLLIN)) {
> +                               errno = EIO;
> +                               n = -1;
> +                       }
> +                       if (n <= 0) {
> +                               io->eof = true;
> +                               return -1;
> +                       }
> +               }
> +               n = read(io->fd, io->buf, io->buf_len);
>
>                 if (n <= 0) {
>                         io->eof = true;
> --
> 2.41.0.rc0.172.g3f132b7071-goog
>

Re: [PATCH v1 1/2] tools api: Add simple timeout to io read

[Arnaldo Carvalho de Melo]

Em Wed, Jun 14, 2023 at 10:36:47AM -0700, Ian Rogers escreveu:
> On Wed, Jun 7, 2023 at 11:20 PM Ian Rogers <irogers@google.com> wrote:
> >
> > In situations like reading from a pipe it can be useful to have a
> > timeout so that the caller doesn't block indefinitely. Implement a
> > simple one based on poll.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> There is overlap in what these patches aim to fix with the 2 submitted
> patches making addr2line more robust:
> https://lore.kernel.org/all/20230613034817.1356114-2-irogers@google.com/
> 
> I think it could be pragmatic to take both of them. Be robust but
> timeout if addr2line doesn't respond for 1s. What do you think?

Agreed, fixed up a minor conflict and applied.

- Arnaldo
 
> Thanks,
> Ian
> 
> > ---
> >  tools/lib/api/io.h | 28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/api/io.h b/tools/lib/api/io.h
> > index d5e8cf0dada0..9fc429d2852d 100644
> > --- a/tools/lib/api/io.h
> > +++ b/tools/lib/api/io.h
> > @@ -8,6 +8,7 @@
> >  #define __API_IO__
> >
> >  #include <errno.h>
> > +#include <poll.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> >  #include <unistd.h>
> > @@ -23,6 +24,8 @@ struct io {
> >         char *end;
> >         /* Currently accessed data pointer. */
> >         char *data;
> > +       /* Read timeout, 0 implies no timeout. */
> > +       int timeout_ms;
> >         /* Set true on when the end of file on read error. */
> >         bool eof;
> >  };
> > @@ -35,6 +38,7 @@ static inline void io__init(struct io *io, int fd,
> >         io->buf = buf;
> >         io->end = buf;
> >         io->data = buf;
> > +       io->timeout_ms = 0;
> >         io->eof = false;
> >  }
> >
> > @@ -47,7 +51,29 @@ static inline int io__get_char(struct io *io)
> >                 return -1;
> >
> >         if (ptr == io->end) {
> > -               ssize_t n = read(io->fd, io->buf, io->buf_len);
> > +               ssize_t n;
> > +
> > +               if (io->timeout_ms != 0) {
> > +                       struct pollfd pfds[] = {
> > +                               {
> > +                                       .fd = io->fd,
> > +                                       .events = POLLIN,
> > +                               },
> > +                       };
> > +
> > +                       n = poll(pfds, 1, io->timeout_ms);
> > +                       if (n == 0)
> > +                               errno = ETIMEDOUT;
> > +                       if (n > 0 && !(pfds[0].revents & POLLIN)) {
> > +                               errno = EIO;
> > +                               n = -1;
> > +                       }
> > +                       if (n <= 0) {
> > +                               io->eof = true;
> > +                               return -1;
> > +                       }
> > +               }
> > +               n = read(io->fd, io->buf, io->buf_len);
> >
> >                 if (n <= 0) {
> >                         io->eof = true;
> > --
> > 2.41.0.rc0.172.g3f132b7071-goog
> >

-- 

- Arnaldo