* [PATCH v1 1/2] tools api: Add simple timeout to io read
@ 2023-06-08 6:18 Ian Rogers
2023-06-08 6:18 ` [PATCH v1 2/2] perf srcline: Add a timeout to reading from addr2line Ian Rogers
2023-06-14 17:36 ` [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
0 siblings, 2 replies; 4+ messages in thread
From: Ian Rogers @ 2023-06-08 6:18 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Yang Jihong, linux-kernel,
linux-perf-users
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v1 2/2] perf srcline: Add a timeout to reading from addr2line
2023-06-08 6:18 [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
@ 2023-06-08 6:18 ` Ian Rogers
2023-06-14 17:36 ` [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
1 sibling, 0 replies; 4+ messages in thread
From: Ian Rogers @ 2023-06-08 6:18 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Yang Jihong, linux-kernel,
linux-perf-users
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/2] tools api: Add simple timeout to io read
2023-06-08 6:18 [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
2023-06-08 6:18 ` [PATCH v1 2/2] perf srcline: Add a timeout to reading from addr2line Ian Rogers
@ 2023-06-14 17:36 ` Ian Rogers
2023-06-14 18:44 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 4+ messages in thread
From: Ian Rogers @ 2023-06-14 17:36 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Yang Jihong, linux-kernel,
linux-perf-users
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1 1/2] tools api: Add simple timeout to io read
2023-06-14 17:36 ` [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
@ 2023-06-14 18:44 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-06-14 18:44 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Adrian Hunter, Yang Jihong,
linux-kernel, linux-perf-users
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-14 18:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-08 6:18 [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
2023-06-08 6:18 ` [PATCH v1 2/2] perf srcline: Add a timeout to reading from addr2line Ian Rogers
2023-06-14 17:36 ` [PATCH v1 1/2] tools api: Add simple timeout to io read Ian Rogers
2023-06-14 18:44 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).