| Message ID | 52161d343abeda6d039d2f0734904a1383a4c62e.1650617571.git.bristot@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] rtla: Remove procps-ng dependency | expand |
On Fri, 22 Apr 2022 12:01:31 +0200 Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > + > +/* > + * procfs_is_workload_pid - check if a procfs entry contains a workload_prefix* comm > + * > + * Check if the procfs entry is a directory of a process, and then check if the > + * process has a comm with the prefix set in char *workload_prefix. As the > + * current users of this function only check for kernel threads, there is no > + * need to check for the threads for the process. > + * > + * Return: True if the proc_entry contains a comm file with workload_prefix*. > + * Otherwise returns false. > + */ > +static int procfs_is_workload_pid(const char *workload_prefix, struct dirent *proc_entry) > +{ > + char comm_path[MAX_PATH], comm[MAX_PATH]; This is probably fine (but there is one issue), but I would have done this a little different. int len = strlen(workload_prefix); char comm[len + 1]; > + int comm_fd, retval; > + char *t_name; > + > + if (proc_entry->d_type != DT_DIR) > + return 0; > + > + if (*proc_entry->d_name == '.') > + return 0; > + > + /* check if the string is a pid */ > + for (t_name = proc_entry->d_name; t_name; t_name++) { > + if (!isdigit(*t_name)) > + break; > + } > + > + if (*t_name != '\0') > + return 0; > + > + snprintf(comm_path, MAX_PATH, "/proc/%s/comm", proc_entry->d_name); > + comm_fd = open(comm_path, O_RDONLY); > + if (comm_fd < 0) > + return 0; > + > + memset(comm, 0, MAX_PATH); No need for the memset. > + retval = read(comm_fd, comm, MAX_PATH); retval = read(comm_fd, comm, len + 1); > + > + close(comm_fd); > + > + if (retval <= 0) > + return 0; if (comm[len] != '\n') return 0; > + > + retval = !strncmp(workload_prefix, comm, strlen(workload_prefix)); What happens if strlen(workload_prefix) is greater than MAX_PATH? ;-) retval = !strncmp(workload_prefix, comm, len); But that's me. If you want to keep this as is, let me know. -- Steve > + if (!retval) > + return 0; > + > + /* comm already have \n */ > + debug_msg("Found workload pid:%s comm:%s", proc_entry->d_name, comm); > + > + return 1; > +} > +
On 4/26/22 03:45, Steven Rostedt wrote: > On Fri, 22 Apr 2022 12:01:31 +0200 > Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > > >> + >> +/* >> + * procfs_is_workload_pid - check if a procfs entry contains a workload_prefix* comm >> + * >> + * Check if the procfs entry is a directory of a process, and then check if the >> + * process has a comm with the prefix set in char *workload_prefix. As the >> + * current users of this function only check for kernel threads, there is no >> + * need to check for the threads for the process. >> + * >> + * Return: True if the proc_entry contains a comm file with workload_prefix*. >> + * Otherwise returns false. >> + */ >> +static int procfs_is_workload_pid(const char *workload_prefix, struct dirent *proc_entry) >> +{ >> + char comm_path[MAX_PATH], comm[MAX_PATH]; > > This is probably fine (but there is one issue), but I would have done this > a little different. > > int len = strlen(workload_prefix); > char comm[len + 1]; > >> + int comm_fd, retval; >> + char *t_name; >> + >> + if (proc_entry->d_type != DT_DIR) >> + return 0; >> + >> + if (*proc_entry->d_name == '.') >> + return 0; >> + >> + /* check if the string is a pid */ >> + for (t_name = proc_entry->d_name; t_name; t_name++) { >> + if (!isdigit(*t_name)) >> + break; >> + } >> + >> + if (*t_name != '\0') >> + return 0; >> + >> + snprintf(comm_path, MAX_PATH, "/proc/%s/comm", proc_entry->d_name); >> + comm_fd = open(comm_path, O_RDONLY); >> + if (comm_fd < 0) >> + return 0; >> + >> + memset(comm, 0, MAX_PATH); > > No need for the memset. > >> + retval = read(comm_fd, comm, MAX_PATH); > > retval = read(comm_fd, comm, len + 1); > >> + >> + close(comm_fd); >> + >> + if (retval <= 0) >> + return 0; > > if (comm[len] != '\n') > return 0; > >> + >> + retval = !strncmp(workload_prefix, comm, strlen(workload_prefix)); > > What happens if strlen(workload_prefix) is greater than MAX_PATH? ;-) That is a concern I had, but we have only two use cases on rtla so far, and they are both bounded: 'timerlat/' 'osnoise/'... I will add a check though. > > retval = !strncmp(workload_prefix, comm, len); > > But that's me. If you want to keep this as is, let me know. I made these changes... but I am loosing the debug_msg() with the comm/pid that matched. I am adding these messages (and I plan to add more) so I can use on testing scripts... At the same time, I can share a char buffer[MAX_PATH] for both the comm and comm_path... I think I will go this way (using a single buffer), adding a warning... and... > -- Steve > > > >> + if (!retval) >> + return 0; also removing ! in the the revtal = !strcmp(...); if (!retval). Thoughts? -- Daniel >> + >> + /* comm already have \n */ >> + debug_msg("Found workload pid:%s comm:%s", proc_entry->d_name, comm); >> + >> + return 1; >> +} >> +
diff --git a/tools/tracing/rtla/Makefile b/tools/tracing/rtla/Makefile index 11fb417abb42..90b8e20d1118 100644 --- a/tools/tracing/rtla/Makefile +++ b/tools/tracing/rtla/Makefile @@ -31,7 +31,7 @@ TRACEFS_HEADERS := $$($(PKG_CONFIG) --cflags libtracefs) CFLAGS := -O -g -DVERSION=\"$(VERSION)\" $(FOPTS) $(MOPTS) $(WOPTS) $(TRACEFS_HEADERS) LDFLAGS := -ggdb -LIBS := $$($(PKG_CONFIG) --libs libtracefs) -lprocps +LIBS := $$($(PKG_CONFIG) --libs libtracefs) SRC := $(wildcard src/*.c) HDR := $(wildcard src/*.h) diff --git a/tools/tracing/rtla/README.txt b/tools/tracing/rtla/README.txt index 6c88446f7e74..7bc0b931ddfd 100644 --- a/tools/tracing/rtla/README.txt +++ b/tools/tracing/rtla/README.txt @@ -13,7 +13,6 @@ following libraries: - libtracefs - libtraceevent - - procps It also depends on python3-docutils to compile man pages. diff --git a/tools/tracing/rtla/src/utils.c b/tools/tracing/rtla/src/utils.c index da2b590edaed..baf9003d5d41 100644 --- a/tools/tracing/rtla/src/utils.c +++ b/tools/tracing/rtla/src/utils.c @@ -3,7 +3,7 @@ * Copyright (C) 2021 Red Hat Inc, Daniel Bristot de Oliveira <bristot@kernel.org> */ -#include <proc/readproc.h> +#include <dirent.h> #include <stdarg.h> #include <stdlib.h> #include <string.h> @@ -262,6 +262,62 @@ int __set_sched_attr(int pid, struct sched_attr *attr) return 0; } + +/* + * procfs_is_workload_pid - check if a procfs entry contains a workload_prefix* comm + * + * Check if the procfs entry is a directory of a process, and then check if the + * process has a comm with the prefix set in char *workload_prefix. As the + * current users of this function only check for kernel threads, there is no + * need to check for the threads for the process. + * + * Return: True if the proc_entry contains a comm file with workload_prefix*. + * Otherwise returns false. + */ +static int procfs_is_workload_pid(const char *workload_prefix, struct dirent *proc_entry) +{ + char comm_path[MAX_PATH], comm[MAX_PATH]; + int comm_fd, retval; + char *t_name; + + if (proc_entry->d_type != DT_DIR) + return 0; + + if (*proc_entry->d_name == '.') + return 0; + + /* check if the string is a pid */ + for (t_name = proc_entry->d_name; t_name; t_name++) { + if (!isdigit(*t_name)) + break; + } + + if (*t_name != '\0') + return 0; + + snprintf(comm_path, MAX_PATH, "/proc/%s/comm", proc_entry->d_name); + comm_fd = open(comm_path, O_RDONLY); + if (comm_fd < 0) + return 0; + + memset(comm, 0, MAX_PATH); + retval = read(comm_fd, comm, MAX_PATH); + + close(comm_fd); + + if (retval <= 0) + return 0; + + retval = !strncmp(workload_prefix, comm, strlen(workload_prefix)); + if (!retval) + return 0; + + /* comm already have \n */ + debug_msg("Found workload pid:%s comm:%s", proc_entry->d_name, comm); + + return 1; +} + /* * set_comm_sched_attr - set sched params to threads starting with char *comm * @@ -272,33 +328,26 @@ int __set_sched_attr(int pid, struct sched_attr *attr) */ int set_comm_sched_attr(const char *comm, struct sched_attr *attr) { - int flags = PROC_FILLCOM | PROC_FILLSTAT; - PROCTAB *ptp; - proc_t task; + struct dirent *proc_entry; + DIR *procfs; int retval; - ptp = openproc(flags); - if (!ptp) { - err_msg("error openproc()\n"); - return -ENOENT; - } + procfs = opendir("/proc"); - memset(&task, 0, sizeof(task)); + while ((proc_entry = readdir(procfs))) { - while (readproc(ptp, &task)) { - retval = strncmp(comm, task.cmd, strlen(comm)); - if (retval) + retval = procfs_is_workload_pid(comm, proc_entry); + if (!retval) continue; - retval = __set_sched_attr(task.tid, attr); + + /* procfs_is_workload_pid confirmed it is a pid */ + retval = __set_sched_attr(atoi(proc_entry->d_name), attr); if (retval) goto out_err; } - - closeproc(ptp); return 0; out_err: - closeproc(ptp); return 1; } diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h index fa08e374870a..4959a3e2c0d5 100644 --- a/tools/tracing/rtla/src/utils.h +++ b/tools/tracing/rtla/src/utils.h @@ -6,6 +6,7 @@ * '18446744073709551615\0' */ #define BUFF_U64_STR_SIZE 24 +#define MAX_PATH 1024 #define container_of(ptr, type, member)({ \ const typeof(((type *)0)->member) *__mptr = (ptr); \
Daniel Wagner reported to me that readproc.h got deprecated. Also, while the procps-ng library was available on Fedora, it was not available on RHEL, which is a piece of evidence that it was not that used. rtla uses procps-ng only to find the PID of the tracers' workload. I used the procps-ng library to avoid reinventing the wheel. But in this case, reinventing the wheel took me less time than the time we already took trying to work around problems. Implement a function that reads /proc/ entries, checking if: - the entry is a directory - the directory name is composed only of digits (PID) - the directory contains the comm file - the comm file contains a comm that matches the tracers' workload prefix. - then return true; otherwise, return false. And use it instead of procps-ng. Cc: John Kacur <jkacur@redhat.com> Cc: Steven Rostedt <rostedt@goodmis.org> Fixes: b1696371d865 ("rtla: Helper functions for rtla") Reported-by: Daniel Wagner <dwagner@suse.de> Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org> --- tools/tracing/rtla/Makefile | 2 +- tools/tracing/rtla/README.txt | 1 - tools/tracing/rtla/src/utils.c | 83 +++++++++++++++++++++++++++------- tools/tracing/rtla/src/utils.h | 1 + 4 files changed, 68 insertions(+), 19 deletions(-)