From patchwork Fri Dec 20 20:19:48 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 37041 Received: (qmail 68019 invoked by alias); 20 Dec 2019 20:19:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 68011 invoked by uid 89); 20 Dec 2019 20:19:56 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-pg1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:autocrypt:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=GRKvICfHuGKjM7MfyuarbHkpYHKXD07ngrm+1sZbrrA=; b=B3d0HrvDuLiu+b5QI3+MK9lhMAYwIcDBOTxIn+WOMnJl2DepWgTlTGohXG+B+8rSMa Cwkm610suQp1L/jQe7MfJSYLMVjEZSDw6rsNjszEChjyfcpDfJ84KWMJUnXKwLV++nlL B4OujYVjlIY/pOFeHsxhVZsashllr3hYxCAsOmvmBuSbumrgIcanRMTiG3zjEz04Qa+n gGzLePCK1rfZKkpfcds1j84Zgz0C/M2Xe5zDlhgDYmf8o3lr7heT/ZMhltuDEd0OEWtK aWeMPWezT8CRqDKjZez1JWvpxfAh7KNRpFQ0/3fhsNRByU9BhdoqR3Pqp+KjNuzHYNxk XdSg== Return-Path: Subject: Re: [PATCH 1/3] support: Add support_process_state_wait To: Florian Weimer Cc: libc-alpha@sourceware.org References: <20191120141535.5303-1-adhemerval.zanella@linaro.org> <87bls3hywz.fsf@oldenburg2.str.redhat.com> From: Adhemerval Zanella Message-ID: <7bb7f49e-084f-cd65-ad02-48660ffeae5c@linaro.org> Date: Fri, 20 Dec 2019 17:19:48 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <87bls3hywz.fsf@oldenburg2.str.redhat.com> On 20/12/2019 11:09, Florian Weimer wrote: > * Adhemerval Zanella: > >> It allows parent process to wait for children state using a polling >> strategy over procfs on Linux. The polling is used over ptrace to >> avoid the need to handle signals on the target pid and to handle some >> system specific limitations (such as YAMA). >> >> The polling has some limitations, such as resource consumption due >> the procfs read in a loop and the lack of synchronization after the >> state is obtained. >> >> The interface idea is to simplify some sleep synchronization in waitid >> tests. > > The goal is to reduce timeouts, I assume, replacing arbitrary two-second > waits. Indeed, I updated the commit message. > >> diff --git a/support/process_state.h b/support/process_state.h >> new file mode 100644 >> index 0000000000..d616f0e3e6 >> --- /dev/null >> +++ b/support/process_state.h >> @@ -0,0 +1,41 @@ > >> +enum process_state_t >> +{ >> + process_state_running = 0x01, /* R (running) */ >> + process_state_sleeping = 0x02, /* S (sleeping) */ >> + process_state_disk_sleep = 0x04, /* D (disk sleep) */ >> + process_state_stopped = 0x08, /* T (stopped) */ >> + process_state_tracing_stop = 0x10, /* t (tracing stop) */ >> + process_state_dead = 0x20, /* X (dead) */ >> + process_state_zombie = 0x40, /* Z (zombie) */ >> + process_state_parked = 0x80, /* P (parked) */ >> +}; > > Maybe drop the reversed _t suffix and add a support_ prefix? Ack. > >> + >> +/* Wait for process PID to reach state STATE. It can be a combination of >> + multiple possible states ('process_state_running | process_state_sleeping') >> + where the function return when any of these state are observed. */ >> +void support_process_state_wait (pid_t pid, enum process_state_t state); > > This interface does not allow to poll for a process not to have a > specific state. I guess we can adjust it once the need arises, so you > can leave it for now. Ok. > >> --- /dev/null >> +++ b/support/support_process_state.c >> @@ -0,0 +1,89 @@ > >> +void >> +support_process_state_wait (pid_t pid, enum process_state_t state) >> +{ >> +#ifdef __linux__ >> + /* For Linux it does a polling check on /proc//status checking on >> + third field. */ >> + >> + /* It mimics the kernel states from fs/proc/array.c */ >> + static struct process_states_t >> + { >> + enum process_state_t s; >> + char v; >> + } process_states[] = { >> + { process_state_running, 'R' }, >> + { process_state_sleeping, 'S' }, >> + { process_state_disk_sleep, 'D' }, >> + { process_state_stopped, 'T' }, >> + { process_state_tracing_stop, 't' }, >> + { process_state_dead, 'X' }, >> + { process_state_zombie, 'Z' }, >> + { process_state_parked, 'P' }, >> + }; > > Should be const. Ack. > >> + char proc_path[sizeof ("/proc/" + 3) * sizeof (pid_t) + sizeof ("/stat") >> + + 1]; >> + snprintf (proc_path, sizeof (proc_path), "/proc/%i/stat", pid); >> + >> + int fd = xopen (proc_path, O_RDONLY, 0600); >> + >> + char statbuf[3 * sizeof (pid_t) /* pid. */ >> + + 2 /* ' (' */ >> + + 64 /* task name. */ >> + + 2 /* ' )' */ >> + + 1 /* , 1 char. */ >> + + 1]; /* Final \0. */ >> + for (;;) >> + { >> + ssize_t ret = read (fd, statbuf, sizeof (statbuf) - 1); >> + TEST_VERIFY (ret >= 0); >> + statbuf[ret] = '\0'; >> + >> + char cur_state; >> + int sret = sscanf (statbuf, "%*i %*s %c", &cur_state); >> + TEST_VERIFY (sret == 1); > > Hmm. Does this work if COMM includes a space? I think it's safer to > parse /proc/PID/status. It does not, I updated to parse /proc/PID/status. > >> + for (size_t i = 0; i < array_length (process_states); ++i) >> + if (state & process_states[i].s && cur_state == process_states[i].v) >> + { >> + close (fd); >> + return; >> + } >> + >> + xlseek (fd, 0, SEEK_SET); >> + nanosleep (&(struct timespec) { 0, 10000000 }, NULL); >> + } >> + >> + close (fd); > > Should this have a fallback timeout? Do you mean if it can not get the process state? Currently support_process_state_wait fails either if it can not open the status file or if it can't find 'State:' field. Should we add a fallback for each case? > > The sleep should probably be usleep or nanosleep, to avoid interfering > with laram. Ok. > > The test looks reasonable. Updated patch below: diff --git a/support/Makefile b/support/Makefile index 23c6d74627..b0d28200af 100644 --- a/support/Makefile +++ b/support/Makefile @@ -56,6 +56,7 @@ libsupport-routines = \ support_format_hostent \ support_format_netent \ support_isolate_in_subprocess \ + support_process_state \ support_ptrace \ support_openpty \ support_paths \ @@ -223,6 +224,7 @@ tests = \ tst-support_capture_subprocess \ tst-support_descriptors \ tst-support_format_dns_packet \ + tst-support-process_state \ tst-support_quote_blob \ tst-support_quote_string \ tst-support_record_failure \ diff --git a/support/process_state.h b/support/process_state.h new file mode 100644 index 0000000000..d983197247 --- /dev/null +++ b/support/process_state.h @@ -0,0 +1,45 @@ +/* Wait for process state. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#ifndef SUPPORT_PROCESS_STATE_H +#define SUPPORT_PROCESS_STATE_H + +#include + +enum support_process_state +{ + support_process_state_running = 0x01, /* R (running). */ + support_process_state_sleeping = 0x02, /* S (sleeping). */ + support_process_state_disk_sleep = 0x04, /* D (disk sleep). */ + support_process_state_stopped = 0x08, /* T (stopped). */ + support_process_state_tracing_stop = 0x10, /* t (tracing stop). */ + support_process_state_dead = 0x20, /* X (dead). */ + support_process_state_zombie = 0x40, /* Z (zombie). */ + support_process_state_parked = 0x80, /* P (parked). */ +}; + +/* Wait for process PID to reach state STATE. It can be a combination of + multiple possible states ('process_state_running | process_state_sleeping') + where the function return when any of these state are observed. + The timeout POLL_TS might be used to wait before polling the PID status + information from the kernel. A NULL value will set to use a default + value (10000000 ns). */ +void support_process_state_wait (pid_t pid, enum support_process_state state, + struct timespec *poll_ts); + +#endif diff --git a/support/support_process_state.c b/support/support_process_state.c new file mode 100644 index 0000000000..16c2c3642e --- /dev/null +++ b/support/support_process_state.c @@ -0,0 +1,102 @@ +/* Wait for process state. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include + +#include + +#include +#include +#include + +/* Returns the character representing the task state from FD. */ +static char find_state (int fd) +{ + char statusbuf[strlen ("Name:\t") + + 64 /* task name. */ + + strlen ("Umask:\t") /* optional. */ + + 4 /* umask size. */ + + strlen ("State:\t") + + 1 /* , 1 char. */ + + 1]; /* Final \0. */ + + ssize_t ret = read (fd, statusbuf, sizeof (statusbuf) - 1); + TEST_VERIFY (ret >= 0); + statusbuf[ret] = '\0'; + + char *statestr = strstr (statusbuf, "State:\t"); + TEST_VERIFY (statestr != NULL); + char state; + int sret = sscanf (statestr, "%*s %c", &state); + TEST_VERIFY (sret == 1); + + return state; +} + +void +support_process_state_wait (pid_t pid, enum support_process_state state, + struct timespec *poll_ts) +{ +#ifdef __linux__ + /* For Linux it does a polling check on /proc//status checking on + third field. */ + + /* It mimics the kernel states from fs/proc/array.c */ + static const struct process_states_t + { + enum support_process_state s; + char v; + } process_states[] = { + { support_process_state_running, 'R' }, + { support_process_state_sleeping, 'S' }, + { support_process_state_disk_sleep, 'D' }, + { support_process_state_stopped, 'T' }, + { support_process_state_tracing_stop, 't' }, + { support_process_state_dead, 'X' }, + { support_process_state_zombie, 'Z' }, + { support_process_state_parked, 'P' }, + }; + + char proc_path[sizeof ("/proc/" + 3) * sizeof (pid_t) + sizeof ("/stat") + + 1]; + snprintf (proc_path, sizeof (proc_path), "/proc/%i/status", pid); + + int fd = xopen (proc_path, O_RDONLY, 0600); + + for (;;) + { + char cur_state = find_state (fd); + + for (size_t i = 0; i < array_length (process_states); ++i) + if (state & process_states[i].s && cur_state == process_states[i].v) + goto found; + + xlseek (fd, 0, SEEK_SET); + + nanosleep (poll_ts ? poll_ts : &(struct timespec) { 0, 10000000 }, NULL); + } + +found: + xclose (fd); +#else + nanosleep (&(struct timespec) { 2, 0 }, NULL); +#endif +} diff --git a/support/tst-support-process_state.c b/support/tst-support-process_state.c new file mode 100644 index 0000000000..d0aa142c76 --- /dev/null +++ b/support/tst-support-process_state.c @@ -0,0 +1,101 @@ +/* Wait for process state tests. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +static void +sigusr1_handler (int signo) +{ +} + +static void +test_child (void) +{ + xsignal (SIGUSR1, sigusr1_handler); + + raise (SIGSTOP); + + TEST_COMPARE (pause (), -1); + TEST_COMPARE (errno, EINTR); + + while (1) + asm (""); +} + +static int +do_test (void) +{ + pid_t pid = xfork (); + if (pid == 0) + { + test_child (); + _exit (127); + } + + /* Adding process_state_tracing_stop ('t') allows the test to work under + trace programs such as ptrace. */ + enum support_process_state stop_state = support_process_state_stopped + | support_process_state_tracing_stop; + + if (test_verbose) + printf ("info: waiting pid %d, state_stopped/state_tracing_stop\n", + (int) pid); + support_process_state_wait (pid, stop_state, NULL); + + if (kill (pid, SIGCONT) != 0) + FAIL_RET ("kill (%d, SIGCONT): %m\n", pid); + + if (test_verbose) + printf ("info: waiting pid %d, state_sleeping\n", (int) pid); + support_process_state_wait (pid, support_process_state_sleeping, NULL); + + if (kill (pid, SIGUSR1) != 0) + FAIL_RET ("kill (%d, SIGUSR1): %m\n", pid); + + if (test_verbose) + printf ("info: waiting pid %d, state_running\n", (int) pid); + support_process_state_wait (pid, support_process_state_running, NULL); + + if (kill (pid, SIGKILL) != 0) + FAIL_RET ("kill (%d, SIGKILL): %m\n", pid); + + if (test_verbose) + printf ("info: waiting pid %d, state_zombie\n", (int) pid); + support_process_state_wait (pid, support_process_state_zombie, NULL); + + siginfo_t info; + int r = waitid (P_PID, pid, &info, WEXITED); + TEST_COMPARE (r, 0); + TEST_COMPARE (info.si_signo, SIGCHLD); + TEST_COMPARE (info.si_code, CLD_KILLED); + TEST_COMPARE (info.si_status, SIGKILL); + TEST_COMPARE (info.si_pid, pid); + + return 0; +} + +#include