* [PATCH v1 1/6] tools build: Add a feature test for getdents64
2023-12-07 5:04 [PATCH v1 0/6] Add io_dir to avoid memory overhead from opendir Ian Rogers
@ 2023-12-07 5:04 ` Ian Rogers
2023-12-07 5:04 ` [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative Ian Rogers
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-12-07 5:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Masami Hiramatsu (Google),
Thomas Richter, Quentin Monnet, Fangrui Song, Kan Liang,
Yanteng Si, K Prateek Nayak, Artem Savkov, James Clark,
Ravi Bangoria, linux-kernel, linux-perf-users
getdents64 may be missing from certain libcs, add a feature test to
determine when such a libc is being used.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/build/Makefile.feature | 1 +
tools/build/feature/Makefile | 4 ++++
tools/build/feature/test-all.c | 5 +++++
tools/build/feature/test-getdents64.c | 12 ++++++++++++
4 files changed, 22 insertions(+)
create mode 100644 tools/build/feature/test-getdents64.c
diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
index 64df118376df..f051d4d8c71c 100644
--- a/tools/build/Makefile.feature
+++ b/tools/build/Makefile.feature
@@ -58,6 +58,7 @@ FEATURE_TESTS_BASIC := \
pthread-attr-setaffinity-np \
pthread-barrier \
reallocarray \
+ getdents64 \
stackprotector-all \
timerfd \
libdw-dwarf-unwind \
diff --git a/tools/build/feature/Makefile b/tools/build/feature/Makefile
index 37722e509eb9..5efe89c3b0a6 100644
--- a/tools/build/feature/Makefile
+++ b/tools/build/feature/Makefile
@@ -21,6 +21,7 @@ FILES= \
test-disassembler-four-args.bin \
test-disassembler-init-styled.bin \
test-reallocarray.bin \
+ test-getdents64.bin \
test-libbfd-liberty.bin \
test-libbfd-liberty-z.bin \
test-cplus-demangle.bin \
@@ -262,6 +263,9 @@ $(OUTPUT)test-disassembler-init-styled.bin:
$(OUTPUT)test-reallocarray.bin:
$(BUILD)
+$(OUTPUT)test-getdents64.bin:
+ $(BUILD)
+
$(OUTPUT)test-libbfd-liberty.bin:
$(CC) $(CFLAGS) -Wall -Werror -o $@ test-libbfd.c -DPACKAGE='"perf"' $(LDFLAGS) -lbfd -ldl -liberty
diff --git a/tools/build/feature/test-all.c b/tools/build/feature/test-all.c
index 6f4bf386a3b5..c65096f75032 100644
--- a/tools/build/feature/test-all.c
+++ b/tools/build/feature/test-all.c
@@ -166,6 +166,10 @@
# include "test-reallocarray.c"
#undef main
+#define main main_test_getdents64
+# include "test-getdents64.c"
+#undef main
+
#define main main_test_disassembler_four_args
# include "test-disassembler-four-args.c"
#undef main
@@ -216,6 +220,7 @@ int main(int argc, char *argv[])
main_test_setns();
main_test_libaio();
main_test_reallocarray();
+ main_test_getdents64();
main_test_disassembler_four_args();
main_test_libzstd();
diff --git a/tools/build/feature/test-getdents64.c b/tools/build/feature/test-getdents64.c
new file mode 100644
index 000000000000..f7c9df1e2f05
--- /dev/null
+++ b/tools/build/feature/test-getdents64.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stddef.h>
+#define _GNU_SOURCE
+#include <dirent.h>
+
+int main(void)
+{
+ char buf[128];
+ return (int)getdents64(0, buf, sizeof(buf));
+}
+
+#undef _GNU_SOURCE
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative
2023-12-07 5:04 [PATCH v1 0/6] Add io_dir to avoid memory overhead from opendir Ian Rogers
2023-12-07 5:04 ` [PATCH v1 1/6] tools build: Add a feature test for getdents64 Ian Rogers
@ 2023-12-07 5:04 ` Ian Rogers
2023-12-11 23:24 ` Namhyung Kim
2023-12-07 5:04 ` [PATCH v1 3/6] perf maps: Switch modules tree walk to io_dir__readdir Ian Rogers
` (3 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2023-12-07 5:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Masami Hiramatsu (Google),
Thomas Richter, Quentin Monnet, Fangrui Song, Kan Liang,
Yanteng Si, K Prateek Nayak, Artem Savkov, James Clark,
Ravi Bangoria, linux-kernel, linux-perf-users
glibc's opendir allocates a minimum of 32kb, when called recursively
for a directory tree the memory consumption can add up - nearly 300kb
during perf start-up when processing modules. Add a stack allocated
variant of readdir sized a little more than 1kb.
As getdents64 may be missing from libc, add support using syscall.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/lib/api/Makefile | 2 +-
tools/lib/api/io_dir.h | 84 ++++++++++++++++++++++++++++++++++++++
tools/perf/Makefile.config | 4 ++
3 files changed, 89 insertions(+), 1 deletion(-)
create mode 100644 tools/lib/api/io_dir.h
diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
index 044860ac1ed1..186aa407de8c 100644
--- a/tools/lib/api/Makefile
+++ b/tools/lib/api/Makefile
@@ -99,7 +99,7 @@ install_lib: $(LIBFILE)
$(call do_install_mkdir,$(libdir_SQ)); \
cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
-HDRS := cpu.h debug.h io.h
+HDRS := cpu.h debug.h io.h io_dir.h
FD_HDRS := fd/array.h
FS_HDRS := fs/fs.h fs/tracing_path.h
INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/api
diff --git a/tools/lib/api/io_dir.h b/tools/lib/api/io_dir.h
new file mode 100644
index 000000000000..9b702497e05c
--- /dev/null
+++ b/tools/lib/api/io_dir.h
@@ -0,0 +1,84 @@
+/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
+/*
+ * Lightweight directory reading library.
+ */
+#ifndef __API_IO_DIR__
+#define __API_IO_DIR__
+
+#include <dirent.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/stat.h>
+
+#ifndef HAVE_GETDENTS64
+#include <sys/syscall.h>
+
+static inline ssize_t getdents64(int fd, void *dirp, size_t count)
+{
+ return syscall(SYS_getdents64, fd, dirp, count);
+}
+#endif
+
+struct io_dirent64 {
+ ino64_t d_ino; /* 64-bit inode number */
+ off64_t d_off; /* 64-bit offset to next structure */
+ unsigned short d_reclen; /* Size of this dirent */
+ unsigned char d_type; /* File type */
+ char d_name[NAME_MAX + 1]; /* Filename (null-terminated) */
+};
+
+struct io_dir {
+ int dirfd;
+ ssize_t available_bytes;
+ struct io_dirent64 *next;
+ struct io_dirent64 buff[4];
+};
+
+static inline void io_dir__init(struct io_dir *iod, int dirfd)
+{
+ iod->dirfd = dirfd;
+ iod->available_bytes = 0;
+}
+
+static inline void io_dir__rewinddir(struct io_dir *iod)
+{
+ lseek(iod->dirfd, 0, SEEK_SET);
+ iod->available_bytes = 0;
+}
+
+static inline struct io_dirent64 *io_dir__readdir(struct io_dir *iod)
+{
+ struct io_dirent64 *entry;
+
+ if (iod->available_bytes <= 0) {
+ ssize_t rc = getdents64(iod->dirfd, iod->buff, sizeof(iod->buff));
+
+ if (rc <= 0)
+ return NULL;
+ iod->available_bytes = rc;
+ iod->next = iod->buff;
+ }
+ entry = iod->next;
+ iod->next = (struct io_dirent64 *)((char *)entry + entry->d_reclen);
+ iod->available_bytes -= entry->d_reclen;
+ return entry;
+}
+
+static inline bool io_dir__is_dir(const struct io_dir *iod, struct io_dirent64 *dent)
+{
+ if (dent->d_type == DT_UNKNOWN) {
+ struct stat st;
+
+ if (fstatat(iod->dirfd, dent->d_name, &st, /*flags=*/0))
+ return false;
+
+ if (S_ISDIR(st.st_mode)) {
+ dent->d_type = DT_DIR;
+ return true;
+ }
+ }
+ return dent->d_type == DT_DIR;
+}
+
+#endif
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index aa55850fbc21..1cef1ab4ddb7 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -357,6 +357,10 @@ ifeq ($(feature-stackprotector-all), 1)
CORE_CFLAGS += -fstack-protector-all
endif
+ifeq ($(feature-getdents64), 1)
+ CFLAGS += -DHAVE_GETDENTS64
+endif
+
ifeq ($(DEBUG),0)
ifeq ($(feature-fortify-source), 1)
CORE_CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative
2023-12-07 5:04 ` [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative Ian Rogers
@ 2023-12-11 23:24 ` Namhyung Kim
2023-12-11 23:54 ` Ian Rogers
0 siblings, 1 reply; 10+ messages in thread
From: Namhyung Kim @ 2023-12-11 23:24 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Masami Hiramatsu (Google), Thomas Richter, Quentin Monnet,
Fangrui Song, Kan Liang, Yanteng Si, K Prateek Nayak,
Artem Savkov, James Clark, Ravi Bangoria, linux-kernel,
linux-perf-users
On Wed, Dec 6, 2023 at 9:04 PM Ian Rogers <irogers@google.com> wrote:
>
> glibc's opendir allocates a minimum of 32kb, when called recursively
> for a directory tree the memory consumption can add up - nearly 300kb
> during perf start-up when processing modules. Add a stack allocated
> variant of readdir sized a little more than 1kb.
>
> As getdents64 may be missing from libc, add support using syscall.
Unfortunately my alpine build has:
In file included from util/machine.c:2:
/build/libapi/include/api/io_dir.h:17:23: error: conflicting types for
'getdents'; have 'ssize_t(int, void *, size_t)' {aka 'long int(int,
void *, long unsigned int)'}
17 | static inline ssize_t getdents64(int fd, void *dirp, size_t count)
| ^~~~~~~~~~
/usr/include/dirent.h:52:5: note: previous declaration of 'getdents'
with type 'int(int, struct dirent *, size_t)' {aka 'int(int, struct
dirent *, long unsigned int)'}
52 | int getdents(int, struct dirent *, size_t);
| ^~~~~~~~
Thanks,
Namhyung
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/lib/api/Makefile | 2 +-
> tools/lib/api/io_dir.h | 84 ++++++++++++++++++++++++++++++++++++++
> tools/perf/Makefile.config | 4 ++
> 3 files changed, 89 insertions(+), 1 deletion(-)
> create mode 100644 tools/lib/api/io_dir.h
>
> diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
> index 044860ac1ed1..186aa407de8c 100644
> --- a/tools/lib/api/Makefile
> +++ b/tools/lib/api/Makefile
> @@ -99,7 +99,7 @@ install_lib: $(LIBFILE)
> $(call do_install_mkdir,$(libdir_SQ)); \
> cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
>
> -HDRS := cpu.h debug.h io.h
> +HDRS := cpu.h debug.h io.h io_dir.h
> FD_HDRS := fd/array.h
> FS_HDRS := fs/fs.h fs/tracing_path.h
> INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/api
> diff --git a/tools/lib/api/io_dir.h b/tools/lib/api/io_dir.h
> new file mode 100644
> index 000000000000..9b702497e05c
> --- /dev/null
> +++ b/tools/lib/api/io_dir.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> +/*
> + * Lightweight directory reading library.
> + */
> +#ifndef __API_IO_DIR__
> +#define __API_IO_DIR__
> +
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <sys/stat.h>
> +
> +#ifndef HAVE_GETDENTS64
> +#include <sys/syscall.h>
> +
> +static inline ssize_t getdents64(int fd, void *dirp, size_t count)
> +{
> + return syscall(SYS_getdents64, fd, dirp, count);
> +}
> +#endif
> +
> +struct io_dirent64 {
> + ino64_t d_ino; /* 64-bit inode number */
> + off64_t d_off; /* 64-bit offset to next structure */
> + unsigned short d_reclen; /* Size of this dirent */
> + unsigned char d_type; /* File type */
> + char d_name[NAME_MAX + 1]; /* Filename (null-terminated) */
> +};
> +
> +struct io_dir {
> + int dirfd;
> + ssize_t available_bytes;
> + struct io_dirent64 *next;
> + struct io_dirent64 buff[4];
> +};
> +
> +static inline void io_dir__init(struct io_dir *iod, int dirfd)
> +{
> + iod->dirfd = dirfd;
> + iod->available_bytes = 0;
> +}
> +
> +static inline void io_dir__rewinddir(struct io_dir *iod)
> +{
> + lseek(iod->dirfd, 0, SEEK_SET);
> + iod->available_bytes = 0;
> +}
> +
> +static inline struct io_dirent64 *io_dir__readdir(struct io_dir *iod)
> +{
> + struct io_dirent64 *entry;
> +
> + if (iod->available_bytes <= 0) {
> + ssize_t rc = getdents64(iod->dirfd, iod->buff, sizeof(iod->buff));
> +
> + if (rc <= 0)
> + return NULL;
> + iod->available_bytes = rc;
> + iod->next = iod->buff;
> + }
> + entry = iod->next;
> + iod->next = (struct io_dirent64 *)((char *)entry + entry->d_reclen);
> + iod->available_bytes -= entry->d_reclen;
> + return entry;
> +}
> +
> +static inline bool io_dir__is_dir(const struct io_dir *iod, struct io_dirent64 *dent)
> +{
> + if (dent->d_type == DT_UNKNOWN) {
> + struct stat st;
> +
> + if (fstatat(iod->dirfd, dent->d_name, &st, /*flags=*/0))
> + return false;
> +
> + if (S_ISDIR(st.st_mode)) {
> + dent->d_type = DT_DIR;
> + return true;
> + }
> + }
> + return dent->d_type == DT_DIR;
> +}
> +
> +#endif
> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> index aa55850fbc21..1cef1ab4ddb7 100644
> --- a/tools/perf/Makefile.config
> +++ b/tools/perf/Makefile.config
> @@ -357,6 +357,10 @@ ifeq ($(feature-stackprotector-all), 1)
> CORE_CFLAGS += -fstack-protector-all
> endif
>
> +ifeq ($(feature-getdents64), 1)
> + CFLAGS += -DHAVE_GETDENTS64
> +endif
> +
> ifeq ($(DEBUG),0)
> ifeq ($(feature-fortify-source), 1)
> CORE_CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
> --
> 2.43.0.rc2.451.g8631bc7472-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative
2023-12-11 23:24 ` Namhyung Kim
@ 2023-12-11 23:54 ` Ian Rogers
2023-12-13 1:33 ` Namhyung Kim
0 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2023-12-11 23:54 UTC (permalink / raw)
To: Namhyung Kim
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Masami Hiramatsu (Google), Thomas Richter, Quentin Monnet,
Fangrui Song, Kan Liang, Yanteng Si, K Prateek Nayak,
Artem Savkov, James Clark, Ravi Bangoria, linux-kernel,
linux-perf-users
On Mon, Dec 11, 2023 at 3:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Dec 6, 2023 at 9:04 PM Ian Rogers <irogers@google.com> wrote:
> >
> > glibc's opendir allocates a minimum of 32kb, when called recursively
> > for a directory tree the memory consumption can add up - nearly 300kb
> > during perf start-up when processing modules. Add a stack allocated
> > variant of readdir sized a little more than 1kb.
> >
> > As getdents64 may be missing from libc, add support using syscall.
>
> Unfortunately my alpine build has:
>
> In file included from util/machine.c:2:
> /build/libapi/include/api/io_dir.h:17:23: error: conflicting types for
> 'getdents'; have 'ssize_t(int, void *, size_t)' {aka 'long int(int,
> void *, long unsigned int)'}
> 17 | static inline ssize_t getdents64(int fd, void *dirp, size_t count)
> | ^~~~~~~~~~
> /usr/include/dirent.h:52:5: note: previous declaration of 'getdents'
> with type 'int(int, struct dirent *, size_t)' {aka 'int(int, struct
> dirent *, long unsigned int)'}
> 52 | int getdents(int, struct dirent *, size_t);
> | ^~~~~~~~
Presumably there is a #define getdents64 getdents .. Could we stop
caring about this version of Alpine linux?
Thanks,
Ian
> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/lib/api/Makefile | 2 +-
> > tools/lib/api/io_dir.h | 84 ++++++++++++++++++++++++++++++++++++++
> > tools/perf/Makefile.config | 4 ++
> > 3 files changed, 89 insertions(+), 1 deletion(-)
> > create mode 100644 tools/lib/api/io_dir.h
> >
> > diff --git a/tools/lib/api/Makefile b/tools/lib/api/Makefile
> > index 044860ac1ed1..186aa407de8c 100644
> > --- a/tools/lib/api/Makefile
> > +++ b/tools/lib/api/Makefile
> > @@ -99,7 +99,7 @@ install_lib: $(LIBFILE)
> > $(call do_install_mkdir,$(libdir_SQ)); \
> > cp -fpR $(LIBFILE) $(DESTDIR)$(libdir_SQ)
> >
> > -HDRS := cpu.h debug.h io.h
> > +HDRS := cpu.h debug.h io.h io_dir.h
> > FD_HDRS := fd/array.h
> > FS_HDRS := fs/fs.h fs/tracing_path.h
> > INSTALL_HDRS_PFX := $(DESTDIR)$(prefix)/include/api
> > diff --git a/tools/lib/api/io_dir.h b/tools/lib/api/io_dir.h
> > new file mode 100644
> > index 000000000000..9b702497e05c
> > --- /dev/null
> > +++ b/tools/lib/api/io_dir.h
> > @@ -0,0 +1,84 @@
> > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
> > +/*
> > + * Lightweight directory reading library.
> > + */
> > +#ifndef __API_IO_DIR__
> > +#define __API_IO_DIR__
> > +
> > +#include <dirent.h>
> > +#include <fcntl.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <sys/stat.h>
> > +
> > +#ifndef HAVE_GETDENTS64
> > +#include <sys/syscall.h>
> > +
> > +static inline ssize_t getdents64(int fd, void *dirp, size_t count)
> > +{
> > + return syscall(SYS_getdents64, fd, dirp, count);
> > +}
> > +#endif
> > +
> > +struct io_dirent64 {
> > + ino64_t d_ino; /* 64-bit inode number */
> > + off64_t d_off; /* 64-bit offset to next structure */
> > + unsigned short d_reclen; /* Size of this dirent */
> > + unsigned char d_type; /* File type */
> > + char d_name[NAME_MAX + 1]; /* Filename (null-terminated) */
> > +};
> > +
> > +struct io_dir {
> > + int dirfd;
> > + ssize_t available_bytes;
> > + struct io_dirent64 *next;
> > + struct io_dirent64 buff[4];
> > +};
> > +
> > +static inline void io_dir__init(struct io_dir *iod, int dirfd)
> > +{
> > + iod->dirfd = dirfd;
> > + iod->available_bytes = 0;
> > +}
> > +
> > +static inline void io_dir__rewinddir(struct io_dir *iod)
> > +{
> > + lseek(iod->dirfd, 0, SEEK_SET);
> > + iod->available_bytes = 0;
> > +}
> > +
> > +static inline struct io_dirent64 *io_dir__readdir(struct io_dir *iod)
> > +{
> > + struct io_dirent64 *entry;
> > +
> > + if (iod->available_bytes <= 0) {
> > + ssize_t rc = getdents64(iod->dirfd, iod->buff, sizeof(iod->buff));
> > +
> > + if (rc <= 0)
> > + return NULL;
> > + iod->available_bytes = rc;
> > + iod->next = iod->buff;
> > + }
> > + entry = iod->next;
> > + iod->next = (struct io_dirent64 *)((char *)entry + entry->d_reclen);
> > + iod->available_bytes -= entry->d_reclen;
> > + return entry;
> > +}
> > +
> > +static inline bool io_dir__is_dir(const struct io_dir *iod, struct io_dirent64 *dent)
> > +{
> > + if (dent->d_type == DT_UNKNOWN) {
> > + struct stat st;
> > +
> > + if (fstatat(iod->dirfd, dent->d_name, &st, /*flags=*/0))
> > + return false;
> > +
> > + if (S_ISDIR(st.st_mode)) {
> > + dent->d_type = DT_DIR;
> > + return true;
> > + }
> > + }
> > + return dent->d_type == DT_DIR;
> > +}
> > +
> > +#endif
> > diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
> > index aa55850fbc21..1cef1ab4ddb7 100644
> > --- a/tools/perf/Makefile.config
> > +++ b/tools/perf/Makefile.config
> > @@ -357,6 +357,10 @@ ifeq ($(feature-stackprotector-all), 1)
> > CORE_CFLAGS += -fstack-protector-all
> > endif
> >
> > +ifeq ($(feature-getdents64), 1)
> > + CFLAGS += -DHAVE_GETDENTS64
> > +endif
> > +
> > ifeq ($(DEBUG),0)
> > ifeq ($(feature-fortify-source), 1)
> > CORE_CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2
> > --
> > 2.43.0.rc2.451.g8631bc7472-goog
> >
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative
2023-12-11 23:54 ` Ian Rogers
@ 2023-12-13 1:33 ` Namhyung Kim
0 siblings, 0 replies; 10+ messages in thread
From: Namhyung Kim @ 2023-12-13 1:33 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter,
Masami Hiramatsu (Google), Thomas Richter, Quentin Monnet,
Fangrui Song, Kan Liang, Yanteng Si, K Prateek Nayak,
Artem Savkov, James Clark, Ravi Bangoria, linux-kernel,
linux-perf-users
On Mon, Dec 11, 2023 at 3:54 PM Ian Rogers <irogers@google.com> wrote:
>
> On Mon, Dec 11, 2023 at 3:25 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, Dec 6, 2023 at 9:04 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > glibc's opendir allocates a minimum of 32kb, when called recursively
> > > for a directory tree the memory consumption can add up - nearly 300kb
> > > during perf start-up when processing modules. Add a stack allocated
> > > variant of readdir sized a little more than 1kb.
> > >
> > > As getdents64 may be missing from libc, add support using syscall.
> >
> > Unfortunately my alpine build has:
> >
> > In file included from util/machine.c:2:
> > /build/libapi/include/api/io_dir.h:17:23: error: conflicting types for
> > 'getdents'; have 'ssize_t(int, void *, size_t)' {aka 'long int(int,
> > void *, long unsigned int)'}
> > 17 | static inline ssize_t getdents64(int fd, void *dirp, size_t count)
> > | ^~~~~~~~~~
> > /usr/include/dirent.h:52:5: note: previous declaration of 'getdents'
> > with type 'int(int, struct dirent *, size_t)' {aka 'int(int, struct
> > dirent *, long unsigned int)'}
> > 52 | int getdents(int, struct dirent *, size_t);
> > | ^~~~~~~~
>
> Presumably there is a #define getdents64 getdents .. Could we stop
> caring about this version of Alpine linux?
Right, there's a #define:
https://git.musl-libc.org/cgit/musl/tree/include/dirent.h#n68
But I'm not sure ignoring Alpine linux is a good idea.
Maybe we can add a #undef right before?
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 3/6] perf maps: Switch modules tree walk to io_dir__readdir
2023-12-07 5:04 [PATCH v1 0/6] Add io_dir to avoid memory overhead from opendir Ian Rogers
2023-12-07 5:04 ` [PATCH v1 1/6] tools build: Add a feature test for getdents64 Ian Rogers
2023-12-07 5:04 ` [PATCH v1 2/6] tools lib api: Add io_dir an allocation free readdir alternative Ian Rogers
@ 2023-12-07 5:04 ` Ian Rogers
2023-12-07 5:04 ` [PATCH v1 4/6] perf pmu: Switch " Ian Rogers
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-12-07 5:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Masami Hiramatsu (Google),
Thomas Richter, Quentin Monnet, Fangrui Song, Kan Liang,
Yanteng Si, K Prateek Nayak, Artem Savkov, James Clark,
Ravi Bangoria, linux-kernel, linux-perf-users
Compared to glibc's opendir/readdir this lowers the max RSS of perf
record by 1.8MB on a Debian machine.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/machine.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index c5de5363b5e7..b6831a1f909d 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -36,6 +36,7 @@
#include <internal/lib.h> // page_size
#include "cgroup.h"
#include "arm64-frame-pointer-unwind-support.h"
+#include <api/io_dir.h>
#include <linux/ctype.h>
#include <symbol/kallsyms.h>
@@ -1551,25 +1552,21 @@ static int maps__set_module_path(struct maps *maps, const char *path, struct kmo
static int maps__set_modules_path_dir(struct maps *maps, const char *dir_name, int depth)
{
- struct dirent *dent;
- DIR *dir = opendir(dir_name);
+ struct io_dirent64 *dent;
+ struct io_dir iod;
int ret = 0;
- if (!dir) {
+ io_dir__init(&iod, open(dir_name, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
+ if (iod.dirfd < 0) {
pr_debug("%s: cannot open %s dir\n", __func__, dir_name);
return -1;
}
- while ((dent = readdir(dir)) != NULL) {
+ while ((dent = io_dir__readdir(&iod)) != NULL) {
char path[PATH_MAX];
- struct stat st;
- /*sshfs might return bad dent->d_type, so we have to stat*/
path__join(path, sizeof(path), dir_name, dent->d_name);
- if (stat(path, &st))
- continue;
-
- if (S_ISDIR(st.st_mode)) {
+ if (io_dir__is_dir(&iod, dent)) {
if (!strcmp(dent->d_name, ".") ||
!strcmp(dent->d_name, ".."))
continue;
@@ -1602,7 +1599,7 @@ static int maps__set_modules_path_dir(struct maps *maps, const char *dir_name, i
}
out:
- closedir(dir);
+ close(iod.dirfd);
return ret;
}
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v1 4/6] perf pmu: Switch to io_dir__readdir
2023-12-07 5:04 [PATCH v1 0/6] Add io_dir to avoid memory overhead from opendir Ian Rogers
` (2 preceding siblings ...)
2023-12-07 5:04 ` [PATCH v1 3/6] perf maps: Switch modules tree walk to io_dir__readdir Ian Rogers
@ 2023-12-07 5:04 ` Ian Rogers
2023-12-07 5:04 ` [PATCH v1 5/6] perf header: Switch mem topology " Ian Rogers
2023-12-07 5:04 ` [PATCH v1 6/6] perf events: Remove scandir in thread synthesis Ian Rogers
5 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-12-07 5:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Masami Hiramatsu (Google),
Thomas Richter, Quentin Monnet, Fangrui Song, Kan Liang,
Yanteng Si, K Prateek Nayak, Artem Savkov, James Clark,
Ravi Bangoria, linux-kernel, linux-perf-users
Avoid DIR allocations when scanning sysfs by using io_dir for the
readdir implementation, that allocates about 1kb on the stack.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/pmu.c | 48 +++++++++++++++++-------------------------
tools/perf/util/pmus.c | 30 ++++++++++----------------
2 files changed, 30 insertions(+), 48 deletions(-)
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 3c9609944a2f..cddb82123a1e 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -12,6 +12,7 @@
#include <stdbool.h>
#include <dirent.h>
#include <api/fs/fs.h>
+#include <api/io_dir.h>
#include <locale.h>
#include <fnmatch.h>
#include <math.h>
@@ -184,19 +185,17 @@ static void perf_pmu_format__load(const struct perf_pmu *pmu, struct perf_pmu_fo
*/
int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd, bool eager_load)
{
- struct dirent *evt_ent;
- DIR *format_dir;
+ struct io_dirent64 *evt_ent;
+ struct io_dir format_dir;
int ret = 0;
- format_dir = fdopendir(dirfd);
- if (!format_dir)
- return -EINVAL;
+ io_dir__init(&format_dir, dirfd);
- while ((evt_ent = readdir(format_dir)) != NULL) {
+ while ((evt_ent = io_dir__readdir(&format_dir)) != NULL) {
struct perf_pmu_format *format;
char *name = evt_ent->d_name;
- if (!strcmp(name, ".") || !strcmp(name, ".."))
+ if (io_dir__is_dir(&format_dir, evt_ent))
continue;
format = perf_pmu__new_format(&pmu->format, name);
@@ -223,7 +222,7 @@ int perf_pmu__format_parse(struct perf_pmu *pmu, int dirfd, bool eager_load)
}
}
- closedir(format_dir);
+ close(format_dir.dirfd);
return ret;
}
@@ -599,8 +598,8 @@ static inline bool pmu_alias_info_file(const char *name)
static int pmu_aliases_parse(struct perf_pmu *pmu)
{
char path[PATH_MAX];
- struct dirent *evt_ent;
- DIR *event_dir;
+ struct io_dirent64 *evt_ent;
+ struct io_dir event_dir;
size_t len;
int fd, dir_fd;
@@ -615,13 +614,9 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
return 0;
}
- event_dir = fdopendir(dir_fd);
- if (!event_dir){
- close (dir_fd);
- return -EINVAL;
- }
+ io_dir__init(&event_dir, dir_fd);
- while ((evt_ent = readdir(event_dir))) {
+ while ((evt_ent = io_dir__readdir(&event_dir))) {
char *name = evt_ent->d_name;
FILE *file;
@@ -651,7 +646,6 @@ static int pmu_aliases_parse(struct perf_pmu *pmu)
fclose(file);
}
- closedir(event_dir);
close (dir_fd);
pmu->sysfs_aliases_loaded = true;
return 0;
@@ -1883,10 +1877,9 @@ static void perf_pmu__del_caps(struct perf_pmu *pmu)
*/
int perf_pmu__caps_parse(struct perf_pmu *pmu)
{
- struct stat st;
char caps_path[PATH_MAX];
- DIR *caps_dir;
- struct dirent *evt_ent;
+ struct io_dir caps_dir;
+ struct io_dirent64 *evt_ent;
int caps_fd;
if (pmu->caps_initialized)
@@ -1897,24 +1890,21 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
if (!perf_pmu__pathname_scnprintf(caps_path, sizeof(caps_path), pmu->name, "caps"))
return -1;
- if (stat(caps_path, &st) < 0) {
+ caps_fd = open(caps_path, O_CLOEXEC | O_DIRECTORY | O_RDONLY);
+ if (caps_fd == -1) {
pmu->caps_initialized = true;
return 0; /* no error if caps does not exist */
}
- caps_dir = opendir(caps_path);
- if (!caps_dir)
- return -EINVAL;
-
- caps_fd = dirfd(caps_dir);
+ io_dir__init(&caps_dir, caps_fd);
- while ((evt_ent = readdir(caps_dir)) != NULL) {
+ while ((evt_ent = io_dir__readdir(&caps_dir)) != NULL) {
char *name = evt_ent->d_name;
char value[128];
FILE *file;
int fd;
- if (!strcmp(name, ".") || !strcmp(name, ".."))
+ if (io_dir__is_dir(&caps_dir, evt_ent))
continue;
fd = openat(caps_fd, name, O_RDONLY);
@@ -1936,7 +1926,7 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
fclose(file);
}
- closedir(caps_dir);
+ close(caps_fd);
pmu->caps_initialized = true;
return pmu->nr_caps;
diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
index ce4931461741..65b23b98666b 100644
--- a/tools/perf/util/pmus.c
+++ b/tools/perf/util/pmus.c
@@ -3,10 +3,10 @@
#include <linux/list_sort.h>
#include <linux/string.h>
#include <linux/zalloc.h>
+#include <api/io_dir.h>
#include <subcmd/pager.h>
#include <sys/types.h>
#include <ctype.h>
-#include <dirent.h>
#include <pthread.h>
#include <string.h>
#include <unistd.h>
@@ -184,8 +184,8 @@ static int pmus_cmp(void *priv __maybe_unused,
static void pmu_read_sysfs(bool core_only)
{
int fd;
- DIR *dir;
- struct dirent *dent;
+ struct io_dir dir;
+ struct io_dirent64 *dent;
if (read_sysfs_all_pmus || (core_only && read_sysfs_core_pmus))
return;
@@ -194,13 +194,9 @@ static void pmu_read_sysfs(bool core_only)
if (fd < 0)
return;
- dir = fdopendir(fd);
- if (!dir) {
- close(fd);
- return;
- }
+ io_dir__init(&dir, fd);
- while ((dent = readdir(dir))) {
+ while ((dent = io_dir__readdir(&dir)) != NULL) {
if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
continue;
if (core_only && !is_pmu_core(dent->d_name))
@@ -209,7 +205,7 @@ static void pmu_read_sysfs(bool core_only)
perf_pmu__find2(fd, dent->d_name);
}
- closedir(dir);
+ close(fd);
if (list_empty(&core_pmus)) {
if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
pr_err("Failure to set up any core PMUs\n");
@@ -563,8 +559,8 @@ bool perf_pmus__supports_extended_type(void)
char *perf_pmus__default_pmu_name(void)
{
int fd;
- DIR *dir;
- struct dirent *dent;
+ struct io_dir dir;
+ struct io_dirent64 *dent;
char *result = NULL;
if (!list_empty(&core_pmus))
@@ -574,13 +570,9 @@ char *perf_pmus__default_pmu_name(void)
if (fd < 0)
return strdup("cpu");
- dir = fdopendir(fd);
- if (!dir) {
- close(fd);
- return strdup("cpu");
- }
+ io_dir__init(&dir, fd);
- while ((dent = readdir(dir))) {
+ while ((dent = io_dir__readdir(&dir)) != NULL) {
if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
continue;
if (is_pmu_core(dent->d_name)) {
@@ -589,7 +581,7 @@ char *perf_pmus__default_pmu_name(void)
}
}
- closedir(dir);
+ close(fd);
return result ?: strdup("cpu");
}
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v1 5/6] perf header: Switch mem topology to io_dir__readdir
2023-12-07 5:04 [PATCH v1 0/6] Add io_dir to avoid memory overhead from opendir Ian Rogers
` (3 preceding siblings ...)
2023-12-07 5:04 ` [PATCH v1 4/6] perf pmu: Switch " Ian Rogers
@ 2023-12-07 5:04 ` Ian Rogers
2023-12-07 5:04 ` [PATCH v1 6/6] perf events: Remove scandir in thread synthesis Ian Rogers
5 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-12-07 5:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Masami Hiramatsu (Google),
Thomas Richter, Quentin Monnet, Fangrui Song, Kan Liang,
Yanteng Si, K Prateek Nayak, Artem Savkov, James Clark,
Ravi Bangoria, linux-kernel, linux-perf-users
Switch memory_node__read and build_mem_topology from opendir/readdir
to io_dir__readdir, with smaller stack allocations. Reduces peak
memory consumption of perf record by 10kb.
Signed-off-by: Ian Rogers <irogers@google.com>
---
| 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
--git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 08cc2febabde..d4a3e28376fd 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -44,6 +44,7 @@
#include "build-id.h"
#include "data.h"
#include <api/fs/fs.h>
+#include <api/io_dir.h>
#include "asm/bug.h"
#include "tool.h"
#include "time-utils.h"
@@ -1341,11 +1342,11 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
{
unsigned int phys, size = 0;
char path[PATH_MAX];
- struct dirent *ent;
- DIR *dir;
+ struct io_dirent64 *ent;
+ struct io_dir dir;
#define for_each_memory(mem, dir) \
- while ((ent = readdir(dir))) \
+ while ((ent = io_dir__readdir(&dir)) != NULL) \
if (strcmp(ent->d_name, ".") && \
strcmp(ent->d_name, "..") && \
sscanf(ent->d_name, "memory%u", &mem) == 1)
@@ -1354,9 +1355,9 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
"%s/devices/system/node/node%lu",
sysfs__mountpoint(), idx);
- dir = opendir(path);
- if (!dir) {
- pr_warning("failed: can't open memory sysfs data\n");
+ io_dir__init(&dir, open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
+ if (dir.dirfd < 0) {
+ pr_warning("failed: can't open memory sysfs data '%s'\n", path);
return -1;
}
@@ -1368,20 +1369,20 @@ static int memory_node__read(struct memory_node *n, unsigned long idx)
n->set = bitmap_zalloc(size);
if (!n->set) {
- closedir(dir);
+ close(dir.dirfd);
return -ENOMEM;
}
n->node = idx;
n->size = size;
- rewinddir(dir);
+ io_dir__rewinddir(&dir);
for_each_memory(phys, dir) {
__set_bit(phys, n->set);
}
- closedir(dir);
+ close(dir.dirfd);
return 0;
}
@@ -1404,8 +1405,8 @@ static int memory_node__sort(const void *a, const void *b)
static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
{
char path[PATH_MAX];
- struct dirent *ent;
- DIR *dir;
+ struct io_dirent64 *ent;
+ struct io_dir dir;
int ret = 0;
size_t cnt = 0, size = 0;
struct memory_node *nodes = NULL;
@@ -1413,14 +1414,14 @@ static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
scnprintf(path, PATH_MAX, "%s/devices/system/node/",
sysfs__mountpoint());
- dir = opendir(path);
- if (!dir) {
+ io_dir__init(&dir, open(path, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
+ if (dir.dirfd < 0) {
pr_debug2("%s: couldn't read %s, does this arch have topology information?\n",
__func__, path);
return -1;
}
- while (!ret && (ent = readdir(dir))) {
+ while (!ret && (ent = io_dir__readdir(&dir))) {
unsigned int idx;
int r;
@@ -1449,7 +1450,7 @@ static int build_mem_topology(struct memory_node **nodesp, u64 *cntp)
cnt += 1;
}
out:
- closedir(dir);
+ close(dir.dirfd);
if (!ret) {
*cntp = cnt;
*nodesp = nodes;
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v1 6/6] perf events: Remove scandir in thread synthesis
2023-12-07 5:04 [PATCH v1 0/6] Add io_dir to avoid memory overhead from opendir Ian Rogers
` (4 preceding siblings ...)
2023-12-07 5:04 ` [PATCH v1 5/6] perf header: Switch mem topology " Ian Rogers
@ 2023-12-07 5:04 ` Ian Rogers
5 siblings, 0 replies; 10+ messages in thread
From: Ian Rogers @ 2023-12-07 5:04 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Ian Rogers, Adrian Hunter, Masami Hiramatsu (Google),
Thomas Richter, Quentin Monnet, Fangrui Song, Kan Liang,
Yanteng Si, K Prateek Nayak, Artem Savkov, James Clark,
Ravi Bangoria, linux-kernel, linux-perf-users
This avoids scanddir reading the directory into memory that's
allocated and instead allocates on the stack.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/synthetic-events.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index a0579c7d7b9e..7cc38f2a0e9e 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -38,6 +38,7 @@
#include <uapi/linux/mman.h> /* To get things like MAP_HUGETLB even on older libc headers */
#include <api/fs/fs.h>
#include <api/io.h>
+#include <api/io_dir.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
@@ -751,10 +752,10 @@ static int __event__synthesize_thread(union perf_event *comm_event,
bool needs_mmap, bool mmap_data)
{
char filename[PATH_MAX];
- struct dirent **dirent;
+ struct io_dir iod;
+ struct io_dirent64 *dent;
pid_t tgid, ppid;
int rc = 0;
- int i, n;
/* special case: only send one comm event using passed in pid */
if (!full) {
@@ -786,16 +787,19 @@ static int __event__synthesize_thread(union perf_event *comm_event,
snprintf(filename, sizeof(filename), "%s/proc/%d/task",
machine->root_dir, pid);
- n = scandir(filename, &dirent, filter_task, NULL);
- if (n < 0)
- return n;
+ io_dir__init(&iod, open(filename, O_CLOEXEC | O_DIRECTORY | O_RDONLY));
+ if (iod.dirfd < 0)
+ return -1;
- for (i = 0; i < n; i++) {
+ while ((dent = io_dir__readdir(&iod)) != NULL) {
char *end;
pid_t _pid;
bool kernel_thread = false;
- _pid = strtol(dirent[i]->d_name, &end, 10);
+ if (!isdigit(dent->d_name[0]))
+ continue;
+
+ _pid = strtol(dent->d_name, &end, 10);
if (*end)
continue;
@@ -829,9 +833,7 @@ static int __event__synthesize_thread(union perf_event *comm_event,
}
}
- for (i = 0; i < n; i++)
- zfree(&dirent[i]);
- free(dirent);
+ close(iod.dirfd);
return rc;
}
--
2.43.0.rc2.451.g8631bc7472-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread