From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id F2DAD10E331 for ; Tue, 16 May 2023 09:33:39 +0000 (UTC) Received: from linux.intel.com (maurocar-mobl2.ger.corp.intel.com [10.252.15.32]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by linux.intel.com (Postfix) with ESMTPS id 36F02580D24 for ; Tue, 16 May 2023 02:33:38 -0700 (PDT) Received: from localhost ([127.0.0.1]) by linux.intel.com with esmtp (Exim 4.96) (envelope-from ) id 1pyr3n-006hDY-34 for igt-dev@lists.freedesktop.org; Tue, 16 May 2023 11:33:35 +0200 Message-ID: Date: Tue, 16 May 2023 11:33:35 +0200 MIME-Version: 1.0 Content-Language: en-US To: igt-dev@lists.freedesktop.org References: <20230515184604.64135-1-kamil.konieczny@linux.intel.com> <20230515184604.64135-2-kamil.konieczny@linux.intel.com> From: Mauro Carvalho Chehab In-Reply-To: <20230515184604.64135-2-kamil.konieczny@linux.intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [PATCH i-g-t 1/2] Use the new procps library libproc2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: Patch series LGTM. Acked-by: Mauro Carvalho Chehab On 5/15/23 20:46, Kamil Konieczny wrote: > From: Craig Small > > Added support for new libproc2. > > [Corrected some errors pointed by checkpatch.pl, > add linux includes in #ifdef __linux__ section] > v2: changed to igt_fork_helper and added error print [Kamil] > v3: removed include pointed by Mauro [Kamil] > > Signed-off-by: Craig Small > --- > lib/igt_aux.c | 246 +++++++++++++++++++++++++++++++++++++++--------- > lib/meson.build | 7 +- > meson.build | 10 +- > 3 files changed, 219 insertions(+), 44 deletions(-) > > diff --git a/lib/igt_aux.c b/lib/igt_aux.c > index 672d7d4b0..4c24b0928 100644 > --- a/lib/igt_aux.c > +++ b/lib/igt_aux.c > @@ -52,8 +52,16 @@ > #include > #include > > -#include > -#include > +#ifdef HAVE_LIBPROCPS > +# include > +#else > +# include > +#endif > + > +#include > +#ifdef __linux__ > +# include > +#endif > > #include "drmtest.h" > #include "i915_drm.h" > @@ -1217,6 +1225,7 @@ void igt_unlock_mem(void) > */ > int igt_is_process_running(const char *comm) > { > +#if HAVE_LIBPROCPS > PROCTAB *proc; > proc_t *proc_info; > bool found = false; > @@ -1235,6 +1244,26 @@ int igt_is_process_running(const char *comm) > > closeproc(proc); > return found; > +#else > + enum pids_item Item[] = { PIDS_CMD }; > + struct pids_info *info = NULL; > + struct pids_stack *stack; > + char *pid_comm; > + bool found = false; > + > + if (procps_pids_new(&info, Item, 1) < 0) > + return false; > + while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) { > + pid_comm = PIDS_VAL(0, str, stack, info); > + if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) { > + found = true; > + break; > + } > + } > + > + procps_pids_unref(&info); > + return found; > +#endif > } > > /** > @@ -1251,6 +1280,7 @@ int igt_is_process_running(const char *comm) > */ > int igt_terminate_process(int sig, const char *comm) > { > +#if HAVE_LIBPROCPS > PROCTAB *proc; > proc_t *proc_info; > int err = 0; > @@ -1272,6 +1302,28 @@ int igt_terminate_process(int sig, const char *comm) > > closeproc(proc); > return err; > +#else > + enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD }; > + struct pids_info *info = NULL; > + struct pids_stack *stack; > + char *pid_comm; > + int pid; > + int err = 0; > + > + if (procps_pids_new(&info, Items, 2) < 0) > + return -errno; > + while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) { > + pid = PIDS_VAL(0, s_int, stack, info); > + pid_comm = PIDS_VAL(1, str, stack, info); > + if (!strncasecmp(pid_comm, comm, strlen(pid_comm))) { > + if (kill(pid, sig) < 0) > + err = -errno; > + break; > + } > + } > + procps_pids_unref(&info); > + return err; > +#endif > } > > struct pinfo { > @@ -1341,9 +1393,9 @@ igt_show_stat_header(void) > } > > static void > -igt_show_stat(proc_t *info, int *state, const char *fn) > +igt_show_stat(const pid_t tid, const char *cmd, int *state, const char *fn) > { > - struct pinfo p = { .pid = info->tid, .comm = info->cmd, .fn = fn }; > + struct pinfo p = { .pid = tid, .comm = cmd, .fn = fn }; > > if (!*state) > igt_show_stat_header(); > @@ -1353,7 +1405,7 @@ igt_show_stat(proc_t *info, int *state, const char *fn) > } > > static void > -__igt_lsof_fds(proc_t *proc_info, int *state, char *proc_path, const char *dir) > +__igt_lsof_fds(const pid_t tid, const char *cmd, int *state, char *proc_path, const char *dir) > { > struct dirent *d; > struct stat st; > @@ -1400,7 +1452,7 @@ again: > dirn = dirname(copy_fd_lnk); > > if (!strncmp(dir, dirn, strlen(dir))) > - igt_show_stat(proc_info, state, fd_lnk); > + igt_show_stat(tid, cmd, state, fd_lnk); > > free(copy_fd_lnk); > free(fd_lnk); > @@ -1416,13 +1468,13 @@ again: > static void > __igt_lsof(const char *dir) > { > - PROCTAB *proc; > - proc_t *proc_info; > - > char path[30]; > char *name_lnk; > struct stat st; > int state = 0; > +#ifdef HAVE_LIBPROCPS > + PROCTAB *proc; > + proc_t *proc_info; > > proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG); > igt_assert(proc != NULL); > @@ -1443,19 +1495,56 @@ __igt_lsof(const char *dir) > name_lnk[read] = '\0'; > > if (!strncmp(dir, name_lnk, strlen(dir))) > - igt_show_stat(proc_info, &state, name_lnk); > + igt_show_stat(proc_info->tid, proc_info->cmd, &state, name_lnk); > > /* check also fd, seems that lsof(8) doesn't look here */ > memset(path, 0, sizeof(path)); > snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid); > > - __igt_lsof_fds(proc_info, &state, path, dir); > + __igt_lsof_fds(proc_info->tid, proc_info->cmd, &state, path, dir); > > free(name_lnk); > freeproc(proc_info); > } > > closeproc(proc); > +#else > + enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD }; > + struct pids_info *info = NULL; > + struct pids_stack *stack; > + > + if (procps_pids_new(&info, Items, 2) < 0) > + return; > + while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) { > + ssize_t read; > + int tid = PIDS_VAL(0, s_int, stack, info); > + char *pid_comm = PIDS_VAL(1, str, stack, info); > + > + /* check current working directory */ > + memset(path, 0, sizeof(path)); > + snprintf(path, sizeof(path), "/proc/%d/cwd", tid); > + > + if (stat(path, &st) == -1) > + continue; > + > + name_lnk = malloc(st.st_size + 1); > + > + igt_assert((read = readlink(path, name_lnk, st.st_size + 1))); > + name_lnk[read] = '\0'; > + > + if (!strncmp(dir, name_lnk, strlen(dir))) > + igt_show_stat(tid, pid_comm, &state, name_lnk); > + > + /* check also fd, seems that lsof(8) doesn't look here */ > + memset(path, 0, sizeof(path)); > + snprintf(path, sizeof(path), "/proc/%d/fd", tid); > + > + __igt_lsof_fds(tid, pid_comm, &state, path, dir); > + > + free(name_lnk); > + } > + procps_pids_unref(&info); > +#endif > } > > /** > @@ -1490,7 +1579,7 @@ igt_lsof(const char *dpath) > free(sanitized); > } > > -static void pulseaudio_unload_module(proc_t *proc_info) > +static void pulseaudio_unload_module(const uid_t euid, const gid_t egid) > { > struct igt_helper_process pa_proc = {}; > char xdg_dir[PATH_MAX]; > @@ -1498,14 +1587,14 @@ static void pulseaudio_unload_module(proc_t *proc_info) > struct passwd *pw; > > igt_fork_helper(&pa_proc) { > - pw = getpwuid(proc_info->euid); > + pw = getpwuid(euid); > homedir = pw->pw_dir; > - snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid); > + snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid); > > igt_info("Request pulseaudio to stop using audio device\n"); > > - setgid(proc_info->egid); > - setuid(proc_info->euid); > + setgid(egid); > + setuid(euid); > clearenv(); > setenv("HOME", homedir, 1); > setenv("XDG_RUNTIME_DIR",xdg_dir, 1); > @@ -1524,10 +1613,13 @@ static void pipewire_reserve_wait(void) > char xdg_dir[PATH_MAX]; > const char *homedir; > struct passwd *pw; > - proc_t *proc_info; > - PROCTAB *proc; > + int tid = 0, euid, egid; > > +#ifdef HAVE_LIBPROCPS > igt_fork_helper(&pw_reserve_proc) { > + proc_t *proc_info; > + PROCTAB *proc; > + > igt_info("Preventing pipewire-pulse to use the audio drivers\n"); > > proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG); > @@ -1540,19 +1632,45 @@ static void pipewire_reserve_wait(void) > } > closeproc(proc); > > + tid = proc_info->tid; > + euid = proc_info->euid; > + egid = proc_info->egid; > + freeproc(proc_info); > +#else > + igt_fork_helper(&pw_reserve_proc) { > + enum pids_item Items[] = { PIDS_ID_PID, PIDS_ID_EUID, PIDS_ID_EGID }; > + enum rel_items { EU_PID, EU_EUID, EU_EGID }; > + struct pids_info *info = NULL; > + struct pids_stack *stack; > + > + igt_info("Preventing pipewire-pulse to use the audio drivers\n"); > + > + if (procps_pids_new(&info, Items, 3) < 0) { > + igt_info("error getting pids: %d\n", errno); > + exit(errno); > + } > + while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) { > + tid = PIDS_VAL(EU_PID, s_int, stack, info); > + if (pipewire_pulse_pid == tid) > + break; > + } > + euid = PIDS_VAL(EU_EUID, s_int, stack, info); > + egid = PIDS_VAL(EU_EGID, s_int, stack, info); > + procps_pids_unref(&info); > +#endif > + > /* Sanity check: if it can't find the process, it means it has gone */ > - if (pipewire_pulse_pid != proc_info->tid) > + if (pipewire_pulse_pid != tid) > exit(0); > > - pw = getpwuid(proc_info->euid); > + pw = getpwuid(euid); > homedir = pw->pw_dir; > - snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", proc_info->euid); > - setgid(proc_info->egid); > - setuid(proc_info->euid); > + snprintf(xdg_dir, sizeof(xdg_dir), "/run/user/%d", euid); > + setgid(egid); > + setuid(euid); > clearenv(); > setenv("HOME", homedir, 1); > setenv("XDG_RUNTIME_DIR",xdg_dir, 1); > - freeproc(proc_info); > > /* > * pw-reserve will run in background. It will only exit when > @@ -1570,9 +1688,7 @@ static void pipewire_reserve_wait(void) > int pipewire_pulse_start_reserve(void) > { > bool is_pw_reserve_running = false; > - proc_t *proc_info; > int attempts = 0; > - PROCTAB *proc; > > if (!pipewire_pulse_pid) > return 0; > @@ -1584,6 +1700,10 @@ int pipewire_pulse_start_reserve(void) > * pipewire version 0.3.50 or upper. > */ > for (attempts = 0; attempts < PIPEWIRE_RESERVE_MAX_TIME; attempts++) { > +#ifdef HAVE_LIBPROCPS > + PROCTAB *proc; > + proc_t *proc_info; > + > usleep(1000); > proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG); > igt_assert(proc != NULL); > @@ -1598,6 +1718,24 @@ int pipewire_pulse_start_reserve(void) > freeproc(proc_info); > } > closeproc(proc); > +#else > + enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD }; > + struct pids_info *info = NULL; > + struct pids_stack *stack; > + > + usleep(1000); > + > + if (procps_pids_new(&info, Items, 2) < 0) > + return 1; > + while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) { > + if (!strcmp(PIDS_VAL(1, str, stack, info), "pw-reserve")) { > + is_pw_reserve_running = true; > + pipewire_pw_reserve_pid = PIDS_VAL(0, s_int, stack, info); > + break; > + } > + } > + procps_pids_unref(&info); > +#endif > if (is_pw_reserve_running) > break; > } > @@ -1645,7 +1783,7 @@ void pipewire_pulse_stop_reserve(void) > * If the check fails, it means that the process can simply be killed. > */ > static int > -__igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path) > +__igt_lsof_audio_and_kill_proc(const pid_t tid, const char *cmd, const uid_t euid, const gid_t egid, char *proc_path) > { > const char *audio_dev = "/dev/snd/"; > char path[PATH_MAX * 2]; > @@ -1670,10 +1808,10 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path) > * 2) unload/unbind the the audio driver(s); > * 3) stop the pw-reserve thread. > */ > - if (!strcmp(proc_info->cmd, "pipewire-pulse")) { > + if (!strcmp(cmd, "pipewire-pulse")) { > igt_info("process %d (%s) is using audio device. Should be requested to stop using them.\n", > - proc_info->tid, proc_info->cmd); > - pipewire_pulse_pid = proc_info->tid; > + tid, cmd); > + pipewire_pulse_pid = tid; > return 0; > } > /* > @@ -1685,9 +1823,9 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path) > * will respawn them. So, just ignore here, they'll honor pw-reserve, > * when the time comes. > */ > - if (!strcmp(proc_info->cmd, "pipewire-media-session")) > + if (!strcmp(cmd, "pipewire-media-session")) > return 0; > - if (!strcmp(proc_info->cmd, "wireplumber")) > + if (!strcmp(cmd, "wireplumber")) > return 0; > > dp = opendir(proc_path); > @@ -1723,22 +1861,22 @@ __igt_lsof_audio_and_kill_proc(proc_t *proc_info, char *proc_path) > * enough to unbind audio modules and won't cause race issues > * with systemd trying to reload it. > */ > - if (!strcmp(proc_info->cmd, "pulseaudio")) { > - pulseaudio_unload_module(proc_info); > + if (!strcmp(cmd, "pulseaudio")) { > + pulseaudio_unload_module(euid, egid); > break; > } > > /* For all other processes, just kill them */ > igt_info("process %d (%s) is using audio device. Should be terminated.\n", > - proc_info->tid, proc_info->cmd); > + tid, cmd); > > - if (kill(proc_info->tid, SIGTERM) < 0) { > + if (kill(tid, SIGTERM) < 0) { > igt_info("Fail to terminate %s (pid: %d) with SIGTERM\n", > - proc_info->cmd, proc_info->tid); > - if (kill(proc_info->tid, SIGABRT) < 0) { > + cmd, tid); > + if (kill(tid, SIGABRT) < 0) { > fail++; > igt_info("Fail to terminate %s (pid: %d) with SIGABRT\n", > - proc_info->cmd, proc_info->tid); > + cmd, tid); > } > } > > @@ -1760,23 +1898,47 @@ int > igt_lsof_kill_audio_processes(void) > { > char path[PATH_MAX]; > + int fail = 0; > + > +#ifdef HAVE_LIBPROCPS > proc_t *proc_info; > PROCTAB *proc; > - int fail = 0; > > proc = openproc(PROC_FILLCOM | PROC_FILLSTAT | PROC_FILLARG); > igt_assert(proc != NULL); > pipewire_pulse_pid = 0; > - > while ((proc_info = readproc(proc, NULL))) { > if (snprintf(path, sizeof(path), "/proc/%d/fd", proc_info->tid) < 1) > fail++; > else > - fail += __igt_lsof_audio_and_kill_proc(proc_info, path); > + fail += __igt_lsof_audio_and_kill_proc(proc_info->tid, proc_info->cmd, proc_info->euid, proc_info->egid, path); > > freeproc(proc_info); > } > closeproc(proc); > +#else > + enum pids_item Items[] = { PIDS_ID_PID, PIDS_CMD, PIDS_ID_EUID, PIDS_ID_EGID }; > + enum rel_items { EU_PID, EU_CMD, EU_EUID, EU_EGID }; > + struct pids_info *info = NULL; > + struct pids_stack *stack; > + pid_t tid; > + > + if (procps_pids_new(&info, Items, 4) < 0) > + return 1; > + while ((stack = procps_pids_get(info, PIDS_FETCH_TASKS_ONLY))) { > + tid = PIDS_VAL(EU_PID, s_int, stack, info); > + > + if (snprintf(path, sizeof(path), "/proc/%d/fd", tid) < 1) > + fail++; > + else > + fail += __igt_lsof_audio_and_kill_proc(tid, > + PIDS_VAL(EU_CMD, str, stack, info), > + PIDS_VAL(EU_EUID, s_int, stack, info), > + PIDS_VAL(EU_EGID, s_int, stack, info), > + path); > + } > + procps_pids_unref(&info); > +#endif > > return fail; > } > diff --git a/lib/meson.build b/lib/meson.build > index 85f100f75..55efdc83b 100644 > --- a/lib/meson.build > +++ b/lib/meson.build > @@ -114,7 +114,6 @@ lib_deps = [ > libdrm, > libdw, > libkmod, > - libprocps, > libudev, > math, > pciaccess, > @@ -178,6 +177,12 @@ if chamelium.found() > lib_sources += 'monitor_edids/monitor_edids_helper.c' > endif > > +if libprocps.found() > + lib_deps += libprocps > +else > + lib_deps += libproc2 > +endif > + > if get_option('srcdir') != '' > srcdir = join_paths(get_option('srcdir'), 'tests') > else > diff --git a/meson.build b/meson.build > index 1c872cc9c..0487158dc 100644 > --- a/meson.build > +++ b/meson.build > @@ -125,7 +125,15 @@ build_info += 'With libdrm: ' + ','.join(libdrm_info) > > pciaccess = dependency('pciaccess', version : '>=0.10') > libkmod = dependency('libkmod') > -libprocps = dependency('libprocps', required : true) > +libprocps = dependency('libprocps', required : false) > +libproc2 = dependency('libproc2', required : false) > +if libprocps.found() > + config.set('HAVE_LIBPROCPS', 1) > +elif libproc2.found() > + config.set('HAVE_LIBPROC2', 1) > +else > + error('Either libprocps or libproc2 is required') > +endif > > libunwind = dependency('libunwind', required : get_option('libunwind')) > build_info += 'With libunwind: @0@'.format(libunwind.found())