Message ID | 8736iuujdu.fsf@xmission.com |
---|---|
State | Not applicable |
Headers |
Received: (qmail 126717 invoked by alias); 25 Jul 2019 17:14:20 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 126697 invoked by uid 89); 25 Jul 2019 17:14:20 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-17.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy=credits, adapt X-HELO: out03.mta.xmission.com From: ebiederm@xmission.com (Eric W. Biederman) To: Arnd Bergmann <arnd@arndb.de> Cc: Rich Felker <dalias@libc.org>, Alistair Francis <alistair23@gmail.com>, Christian Brauner <christian@brauner.io>, Linus Torvalds <torvalds@linux-foundation.org>, Alistair Francis <alistair.francis@wdc.com>, GNU C Library <libc-alpha@sourceware.org>, Adhemerval Zanella <adhemerval.zanella@linaro.org>, Florian Weimer <fweimer@redhat.com>, Palmer Dabbelt <palmer@sifive.com>, macro@wdc.com, Zong Li <zongbox@gmail.com>, Andrew Morton <akpm@linux-foundation.org>, Al Viro <viro@zeniv.linux.org.uk>, "H. Peter Anvin" <hpa@zytor.com> References: <CAK8P3a3wgavtarKxSYJGL0ME9KRZ8UsUAZw+Y5J8WpG1GQ+=mw@mail.gmail.com> <87muh79yt2.fsf@xmission.com> <CAHk-=wjtGaJFceL+YB6=mTxQCvyNvBTavqgGTm-d5FA9xLQ0Cw@mail.gmail.com> <87blxn83sk.fsf@xmission.com> <20190721232336.GA30851@brightrain.aerifal.cx> <87k1c962ml.fsf@xmission.com> <CAK8P3a0jOO8dDK+w0N_RvgUHiW7=i_ak9AyFvH61wqUusL3Drw@mail.gmail.com> <20190723082857.kf2go2vfvnu7q7zd@brauner.io> <CAK8P3a26KBRvJHyWkK0J8FGXQn4jHL2QG10oBGSLidG95xQxrw@mail.gmail.com> <CAKmqyKOXJxJq5-ktLc=oY7ooNj6-X9PotHNM=-xG95iPCAAmjQ@mail.gmail.com> <20190725044009.GJ1506@brightrain.aerifal.cx> <CAK8P3a1pjB8SMFQK0MUom0m5fCSAdVZFtJyeUkCLW-u-RHC3oA@mail.gmail.com> Date: Thu, 25 Jul 2019 12:14:05 -0500 In-Reply-To: <CAK8P3a1pjB8SMFQK0MUom0m5fCSAdVZFtJyeUkCLW-u-RHC3oA@mail.gmail.com> (Arnd Bergmann's message of "Thu, 25 Jul 2019 15:15:35 +0200") Message-ID: <8736iuujdu.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1hqhJk-0002ot-2g; ; ; mid=<8736iuujdu.fsf@xmission.com>; ; ; hst=in01.mta.xmission.com; ; ; ip=68.227.160.95; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX1/epApTIrNUeJuHSK0GivkhPJ2IAL+8Cl8= X-SA-Exim-Connect-IP: 68.227.160.95 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-DCC: XMission; sa06 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Arnd Bergmann <arnd@arndb.de> X-Spam-Relay-Country: X-Spam-Timing: total 706 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 4.4 (0.6%), b_tie_ro: 3.2 (0.5%), parse: 0.92 (0.1%), extract_message_metadata: 20 (2.9%), get_uri_detail_list: 3.1 (0.4%), tests_pri_-1000: 22 (3.1%), tests_pri_-950: 1.28 (0.2%), tests_pri_-900: 1.13 (0.2%), tests_pri_-90: 35 (5.0%), check_bayes: 34 (4.8%), b_tokenize: 12 (1.7%), b_tok_get_all: 12 (1.7%), b_comp_prob: 3.4 (0.5%), b_tok_touch_all: 4.2 (0.6%), b_finish: 0.60 (0.1%), tests_pri_0: 607 (86.0%), check_dkim_signature: 0.55 (0.1%), check_dkim_adsp: 3.0 (0.4%), poll_dns_idle: 0.30 (0.0%), tests_pri_10: 2.3 (0.3%), tests_pri_500: 8 (1.1%), rewrite_mail: 0.00 (0.0%) Subject: Re: [RFC v3 03/23] sysdeps/wait: Use waitid if avaliable X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) |
Commit Message
Eric W. Biederman
July 25, 2019, 5:14 p.m. UTC
Arnd Bergmann <arnd@arndb.de> writes: > On Thu, Jul 25, 2019 at 6:40 AM Rich Felker <dalias@libc.org> wrote: >> On Wed, Jul 24, 2019 at 05:04:53PM -0700, Alistair Francis wrote: >> > On Tue, Jul 23, 2019 at 1:45 AM Arnd Bergmann <arnd@arndb.de> wrote: >> > > >> > > Sounds good to me, the debate over what rusage to use should not hold >> > > up the review of the rest of that syscall. >> > >> > I'm unclear what the final decision is here. What is the solution are >> > we going to have wait4() or add P_PROCESS_PGID to waitid()? >> > >> > As well as that what is the solution to current implementations? If we >> > add wait4() then there isn't an issue (and I can drop this patch) but >> > if we add P_PROCESS_PGID then we will need a way to handle kernels >> > with waitid() but no P_PROCESS_PGID. Although my new plan is to only >> > use the waitid syscall if we don't have waitpid or wait4 so it seems >> > like this will only affect RV32 for the time being. >> >> I would really like some indication which solution will be taken, >> since it impacts choices that will need to be made in musl very soon. >> My favorite outcome would be bringing back wait4 for rv32 (and >> no-time32 archs in general) *and* adding P_PROCESS_PGID. In the short >> term, just using wait4 would be the simplest and cleanest for us (same >> as all other archs, no extra case to deal with), but in the long term >> there may be value in having rusage that can represent more than 68 >> cpu-years spent by a process (seems plausible with large numbers of >> cores). > > Based on the feedback from Linus and Eric, the most likely outcome > at the moment seems to be an extension of waitid() to allow > P_PGID with id=0 like BSD does, and not bring back wait4() or > add P_PROCESS_PGID. > > So far, I don't think anyone has proposed an actual kernel patch. > I was hoping that Eric would do it, but I could also send it if he's > otherwise busy. So here is what I am looking at. It still needs to be tested and the description needs to be improved so that it properly credits everyone. However I think I have the major stroeks correct. From: "Eric W. Biederman" <ebiederm@xmission.com> Date: Tue, 23 Jul 2019 07:44:46 -0500 Subject: [PATCH] waitid: Add support for waiting for the current process group It was recently discovered that the linux version of waitid is not a superset of the other wait functions because it does not include support for waiting for the current process group. This has two downsides. An extra system call is needed to get the current process group, and a signal could come in between the system call that retrieved the process gorup and the call to waitid that changes the current process group. Allow userspace to avoid both of those issues by defining idtype == P_PGID and id == 0 to mean wait for the caller's process group at the time of the call. Arguments can be made for using a different choice of idtype and id for this case but the BSDs already use this P_PGID and 0 to indicate waiting for the current process's process group. So be nice to user space programmers and don't introduce an unnecessary incompatibility. Some people have noted that the posix description is that waitpid will wait for the current process group, and that in the presence of pthreads that process group can change. To get clarity on this issue I looked at XNU, FreeBSD, and Luminos. All of those flavors of unix waited for the current process group at the time of call and as written could not adapt to the process group changing after the call. At one point Linux did adapt to the current process group changing but that stopped in 161550d74c07 ("pid: sys_wait... fixes"). It has been over 11 years since Linux has that behavior, no programs that fail with the change in behavior have been reported, and I could not find any other unix that does this. So I think it is safe to clarify the definition of current process group, to current process group at the time of the wait function. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- kernel/exit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Comments
On July 25, 2019 7:14:05 PM GMT+02:00, ebiederm@xmission.com wrote: >Arnd Bergmann <arnd@arndb.de> writes: > >> On Thu, Jul 25, 2019 at 6:40 AM Rich Felker <dalias@libc.org> wrote: >>> On Wed, Jul 24, 2019 at 05:04:53PM -0700, Alistair Francis wrote: >>> > On Tue, Jul 23, 2019 at 1:45 AM Arnd Bergmann <arnd@arndb.de> >wrote: >>> > > >>> > > Sounds good to me, the debate over what rusage to use should not >hold >>> > > up the review of the rest of that syscall. >>> > >>> > I'm unclear what the final decision is here. What is the solution >are >>> > we going to have wait4() or add P_PROCESS_PGID to waitid()? >>> > >>> > As well as that what is the solution to current implementations? >If we >>> > add wait4() then there isn't an issue (and I can drop this patch) >but >>> > if we add P_PROCESS_PGID then we will need a way to handle kernels >>> > with waitid() but no P_PROCESS_PGID. Although my new plan is to >only >>> > use the waitid syscall if we don't have waitpid or wait4 so it >seems >>> > like this will only affect RV32 for the time being. >>> >>> I would really like some indication which solution will be taken, >>> since it impacts choices that will need to be made in musl very >soon. >>> My favorite outcome would be bringing back wait4 for rv32 (and >>> no-time32 archs in general) *and* adding P_PROCESS_PGID. In the >short >>> term, just using wait4 would be the simplest and cleanest for us >(same >>> as all other archs, no extra case to deal with), but in the long >term >>> there may be value in having rusage that can represent more than 68 >>> cpu-years spent by a process (seems plausible with large numbers of >>> cores). >> >> Based on the feedback from Linus and Eric, the most likely outcome >> at the moment seems to be an extension of waitid() to allow >> P_PGID with id=0 like BSD does, and not bring back wait4() or >> add P_PROCESS_PGID. >> >> So far, I don't think anyone has proposed an actual kernel patch. >> I was hoping that Eric would do it, but I could also send it if he's >> otherwise busy. > >So here is what I am looking at. It still needs to be tested >and the description needs to be improved so that it properly credits >everyone. However I think I have the major stroeks correct. > >From: "Eric W. Biederman" <ebiederm@xmission.com> >Date: Tue, 23 Jul 2019 07:44:46 -0500 >Subject: [PATCH] waitid: Add support for waiting for the current >process group > >It was recently discovered that the linux version of waitid is not a >superset of the other wait functions because it does not include >support for waiting for the current process group. This has two >downsides. An extra system call is needed to get the current process >group, and a signal could come in between the system call that >retrieved the process gorup and the call to waitid that changes the >current process group. > >Allow userspace to avoid both of those issues by defining >idtype == P_PGID and id == 0 to mean wait for the caller's process >group at the time of the call. > >Arguments can be made for using a different choice of idtype and id >for this case but the BSDs already use this P_PGID and 0 to indicate >waiting for the current process's process group. So be nice to user >space programmers and don't introduce an unnecessary incompatibility. > >Some people have noted that the posix description is that >waitpid will wait for the current process group, and that in >the presence of pthreads that process group can change. To get >clarity on this issue I looked at XNU, FreeBSD, and Luminos. All of >those flavors of unix waited for the current process group at the >time of call and as written could not adapt to the process group >changing after the call. > >At one point Linux did adapt to the current process group changing but >that stopped in 161550d74c07 ("pid: sys_wait... fixes"). It has been >over 11 years since Linux has that behavior, no programs that fail >with the change in behavior have been reported, and I could not >find any other unix that does this. So I think it is safe to clarify >the definition of current process group, to current process group >at the time of the wait function. > >Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >--- > kernel/exit.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/kernel/exit.c b/kernel/exit.c >index a75b6a7f458a..3d86930f035e 100644 >--- a/kernel/exit.c >+++ b/kernel/exit.c >@@ -1577,14 +1577,16 @@ static long kernel_waitid(int which, pid_t >upid, struct waitid_info *infop, > break; > case P_PGID: > type = PIDTYPE_PGID; >- if (upid <= 0) >+ if (upid < 0) > return -EINVAL; >+ if (upid == 0) >+ pid = get_pid(task_pgrp(current)); > break; > default: > return -EINVAL; > } > >- if (type < PIDTYPE_MAX) >+ if ((type < PIDTYPE_MAX) && !pid) > pid = find_get_pid(upid); > > wo.wo_type = type; Eric, mind if I send this out alongside the P_PIDFD patchset and put in a test for it? Christian
On Thu, Jul 25, 2019 at 10:14 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > Arnd Bergmann <arnd@arndb.de> writes: > > > On Thu, Jul 25, 2019 at 6:40 AM Rich Felker <dalias@libc.org> wrote: > >> On Wed, Jul 24, 2019 at 05:04:53PM -0700, Alistair Francis wrote: > >> > On Tue, Jul 23, 2019 at 1:45 AM Arnd Bergmann <arnd@arndb.de> wrote: > >> > > > >> > > Sounds good to me, the debate over what rusage to use should not hold > >> > > up the review of the rest of that syscall. > >> > > >> > I'm unclear what the final decision is here. What is the solution are > >> > we going to have wait4() or add P_PROCESS_PGID to waitid()? > >> > > >> > As well as that what is the solution to current implementations? If we > >> > add wait4() then there isn't an issue (and I can drop this patch) but > >> > if we add P_PROCESS_PGID then we will need a way to handle kernels > >> > with waitid() but no P_PROCESS_PGID. Although my new plan is to only > >> > use the waitid syscall if we don't have waitpid or wait4 so it seems > >> > like this will only affect RV32 for the time being. > >> > >> I would really like some indication which solution will be taken, > >> since it impacts choices that will need to be made in musl very soon. > >> My favorite outcome would be bringing back wait4 for rv32 (and > >> no-time32 archs in general) *and* adding P_PROCESS_PGID. In the short > >> term, just using wait4 would be the simplest and cleanest for us (same > >> as all other archs, no extra case to deal with), but in the long term > >> there may be value in having rusage that can represent more than 68 > >> cpu-years spent by a process (seems plausible with large numbers of > >> cores). > > > > Based on the feedback from Linus and Eric, the most likely outcome > > at the moment seems to be an extension of waitid() to allow > > P_PGID with id=0 like BSD does, and not bring back wait4() or > > add P_PROCESS_PGID. > > > > So far, I don't think anyone has proposed an actual kernel patch. > > I was hoping that Eric would do it, but I could also send it if he's > > otherwise busy. > > So here is what I am looking at. It still needs to be tested > and the description needs to be improved so that it properly credits > everyone. However I think I have the major stroeks correct. > > From: "Eric W. Biederman" <ebiederm@xmission.com> > Date: Tue, 23 Jul 2019 07:44:46 -0500 > Subject: [PATCH] waitid: Add support for waiting for the current process group > > It was recently discovered that the linux version of waitid is not a > superset of the other wait functions because it does not include > support for waiting for the current process group. This has two > downsides. An extra system call is needed to get the current process > group, and a signal could come in between the system call that > retrieved the process gorup and the call to waitid that changes the > current process group. > > Allow userspace to avoid both of those issues by defining > idtype == P_PGID and id == 0 to mean wait for the caller's process > group at the time of the call. > > Arguments can be made for using a different choice of idtype and id > for this case but the BSDs already use this P_PGID and 0 to indicate > waiting for the current process's process group. So be nice to user > space programmers and don't introduce an unnecessary incompatibility. > > Some people have noted that the posix description is that > waitpid will wait for the current process group, and that in > the presence of pthreads that process group can change. To get > clarity on this issue I looked at XNU, FreeBSD, and Luminos. All of > those flavors of unix waited for the current process group at the > time of call and as written could not adapt to the process group > changing after the call. > > At one point Linux did adapt to the current process group changing but > that stopped in 161550d74c07 ("pid: sys_wait... fixes"). It has been > over 11 years since Linux has that behavior, no programs that fail > with the change in behavior have been reported, and I could not > find any other unix that does this. So I think it is safe to clarify > the definition of current process group, to current process group > at the time of the wait function. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Will this land in 5.3? Also will it be back ported to stable kernels? Do you mind keeping me in CC when you send the patch? Alistair > --- > kernel/exit.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/exit.c b/kernel/exit.c > index a75b6a7f458a..3d86930f035e 100644 > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -1577,14 +1577,16 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop, > break; > case P_PGID: > type = PIDTYPE_PGID; > - if (upid <= 0) > + if (upid < 0) > return -EINVAL; > + if (upid == 0) > + pid = get_pid(task_pgrp(current)); > break; > default: > return -EINVAL; > } > > - if (type < PIDTYPE_MAX) > + if ((type < PIDTYPE_MAX) && !pid) > pid = find_get_pid(upid); > > wo.wo_type = type; > -- > 2.21.0.dirty >
On Thu, Jul 25, 2019 at 10:30 AM Christian Brauner <christian@brauner.io> wrote: > > On July 25, 2019 7:14:05 PM GMT+02:00, ebiederm@xmission.com wrote: > >Arnd Bergmann <arnd@arndb.de> writes: > > > >> On Thu, Jul 25, 2019 at 6:40 AM Rich Felker <dalias@libc.org> wrote: > >>> On Wed, Jul 24, 2019 at 05:04:53PM -0700, Alistair Francis wrote: > >>> > On Tue, Jul 23, 2019 at 1:45 AM Arnd Bergmann <arnd@arndb.de> > >wrote: > >>> > > > >>> > > Sounds good to me, the debate over what rusage to use should not > >hold > >>> > > up the review of the rest of that syscall. > >>> > > >>> > I'm unclear what the final decision is here. What is the solution > >are > >>> > we going to have wait4() or add P_PROCESS_PGID to waitid()? > >>> > > >>> > As well as that what is the solution to current implementations? > >If we > >>> > add wait4() then there isn't an issue (and I can drop this patch) > >but > >>> > if we add P_PROCESS_PGID then we will need a way to handle kernels > >>> > with waitid() but no P_PROCESS_PGID. Although my new plan is to > >only > >>> > use the waitid syscall if we don't have waitpid or wait4 so it > >seems > >>> > like this will only affect RV32 for the time being. > >>> > >>> I would really like some indication which solution will be taken, > >>> since it impacts choices that will need to be made in musl very > >soon. > >>> My favorite outcome would be bringing back wait4 for rv32 (and > >>> no-time32 archs in general) *and* adding P_PROCESS_PGID. In the > >short > >>> term, just using wait4 would be the simplest and cleanest for us > >(same > >>> as all other archs, no extra case to deal with), but in the long > >term > >>> there may be value in having rusage that can represent more than 68 > >>> cpu-years spent by a process (seems plausible with large numbers of > >>> cores). > >> > >> Based on the feedback from Linus and Eric, the most likely outcome > >> at the moment seems to be an extension of waitid() to allow > >> P_PGID with id=0 like BSD does, and not bring back wait4() or > >> add P_PROCESS_PGID. > >> > >> So far, I don't think anyone has proposed an actual kernel patch. > >> I was hoping that Eric would do it, but I could also send it if he's > >> otherwise busy. > > > >So here is what I am looking at. It still needs to be tested > >and the description needs to be improved so that it properly credits > >everyone. However I think I have the major stroeks correct. > > > >From: "Eric W. Biederman" <ebiederm@xmission.com> > >Date: Tue, 23 Jul 2019 07:44:46 -0500 > >Subject: [PATCH] waitid: Add support for waiting for the current > >process group > > > >It was recently discovered that the linux version of waitid is not a > >superset of the other wait functions because it does not include > >support for waiting for the current process group. This has two > >downsides. An extra system call is needed to get the current process > >group, and a signal could come in between the system call that > >retrieved the process gorup and the call to waitid that changes the > >current process group. > > > >Allow userspace to avoid both of those issues by defining > >idtype == P_PGID and id == 0 to mean wait for the caller's process > >group at the time of the call. > > > >Arguments can be made for using a different choice of idtype and id > >for this case but the BSDs already use this P_PGID and 0 to indicate > >waiting for the current process's process group. So be nice to user > >space programmers and don't introduce an unnecessary incompatibility. > > > >Some people have noted that the posix description is that > >waitpid will wait for the current process group, and that in > >the presence of pthreads that process group can change. To get > >clarity on this issue I looked at XNU, FreeBSD, and Luminos. All of > >those flavors of unix waited for the current process group at the > >time of call and as written could not adapt to the process group > >changing after the call. > > > >At one point Linux did adapt to the current process group changing but > >that stopped in 161550d74c07 ("pid: sys_wait... fixes"). It has been > >over 11 years since Linux has that behavior, no programs that fail > >with the change in behavior have been reported, and I could not > >find any other unix that does this. So I think it is safe to clarify > >the definition of current process group, to current process group > >at the time of the wait function. > > > >Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > >--- > > kernel/exit.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > >diff --git a/kernel/exit.c b/kernel/exit.c > >index a75b6a7f458a..3d86930f035e 100644 > >--- a/kernel/exit.c > >+++ b/kernel/exit.c > >@@ -1577,14 +1577,16 @@ static long kernel_waitid(int which, pid_t > >upid, struct waitid_info *infop, > > break; > > case P_PGID: > > type = PIDTYPE_PGID; > >- if (upid <= 0) > >+ if (upid < 0) > > return -EINVAL; > >+ if (upid == 0) > >+ pid = get_pid(task_pgrp(current)); > > break; > > default: > > return -EINVAL; > > } > > > >- if (type < PIDTYPE_MAX) > >+ if ((type < PIDTYPE_MAX) && !pid) > > pid = find_get_pid(upid); > > > > wo.wo_type = type; > > Eric, mind if I send this out alongside the P_PIDFD patchset and put in a test for it? Was this ever sent? Alistair > > Christian
On Tue, Aug 13, 2019 at 03:22:02PM -0700, Alistair Francis wrote: > On Thu, Jul 25, 2019 at 10:30 AM Christian Brauner <christian@brauner.io> wrote: > > > > On July 25, 2019 7:14:05 PM GMT+02:00, ebiederm@xmission.com wrote: > > >Arnd Bergmann <arnd@arndb.de> writes: > > > > > >> On Thu, Jul 25, 2019 at 6:40 AM Rich Felker <dalias@libc.org> wrote: > > >>> On Wed, Jul 24, 2019 at 05:04:53PM -0700, Alistair Francis wrote: > > >>> > On Tue, Jul 23, 2019 at 1:45 AM Arnd Bergmann <arnd@arndb.de> > > >wrote: > > >>> > > > > >>> > > Sounds good to me, the debate over what rusage to use should not > > >hold > > >>> > > up the review of the rest of that syscall. > > >>> > > > >>> > I'm unclear what the final decision is here. What is the solution > > >are > > >>> > we going to have wait4() or add P_PROCESS_PGID to waitid()? > > >>> > > > >>> > As well as that what is the solution to current implementations? > > >If we > > >>> > add wait4() then there isn't an issue (and I can drop this patch) > > >but > > >>> > if we add P_PROCESS_PGID then we will need a way to handle kernels > > >>> > with waitid() but no P_PROCESS_PGID. Although my new plan is to > > >only > > >>> > use the waitid syscall if we don't have waitpid or wait4 so it > > >seems > > >>> > like this will only affect RV32 for the time being. > > >>> > > >>> I would really like some indication which solution will be taken, > > >>> since it impacts choices that will need to be made in musl very > > >soon. > > >>> My favorite outcome would be bringing back wait4 for rv32 (and > > >>> no-time32 archs in general) *and* adding P_PROCESS_PGID. In the > > >short > > >>> term, just using wait4 would be the simplest and cleanest for us > > >(same > > >>> as all other archs, no extra case to deal with), but in the long > > >term > > >>> there may be value in having rusage that can represent more than 68 > > >>> cpu-years spent by a process (seems plausible with large numbers of > > >>> cores). > > >> > > >> Based on the feedback from Linus and Eric, the most likely outcome > > >> at the moment seems to be an extension of waitid() to allow > > >> P_PGID with id=0 like BSD does, and not bring back wait4() or > > >> add P_PROCESS_PGID. > > >> > > >> So far, I don't think anyone has proposed an actual kernel patch. > > >> I was hoping that Eric would do it, but I could also send it if he's > > >> otherwise busy. > > > > > >So here is what I am looking at. It still needs to be tested > > >and the description needs to be improved so that it properly credits > > >everyone. However I think I have the major stroeks correct. > > > > > >From: "Eric W. Biederman" <ebiederm@xmission.com> > > >Date: Tue, 23 Jul 2019 07:44:46 -0500 > > >Subject: [PATCH] waitid: Add support for waiting for the current > > >process group > > > > > >It was recently discovered that the linux version of waitid is not a > > >superset of the other wait functions because it does not include > > >support for waiting for the current process group. This has two > > >downsides. An extra system call is needed to get the current process > > >group, and a signal could come in between the system call that > > >retrieved the process gorup and the call to waitid that changes the > > >current process group. > > > > > >Allow userspace to avoid both of those issues by defining > > >idtype == P_PGID and id == 0 to mean wait for the caller's process > > >group at the time of the call. > > > > > >Arguments can be made for using a different choice of idtype and id > > >for this case but the BSDs already use this P_PGID and 0 to indicate > > >waiting for the current process's process group. So be nice to user > > >space programmers and don't introduce an unnecessary incompatibility. > > > > > >Some people have noted that the posix description is that > > >waitpid will wait for the current process group, and that in > > >the presence of pthreads that process group can change. To get > > >clarity on this issue I looked at XNU, FreeBSD, and Luminos. All of > > >those flavors of unix waited for the current process group at the > > >time of call and as written could not adapt to the process group > > >changing after the call. > > > > > >At one point Linux did adapt to the current process group changing but > > >that stopped in 161550d74c07 ("pid: sys_wait... fixes"). It has been > > >over 11 years since Linux has that behavior, no programs that fail > > >with the change in behavior have been reported, and I could not > > >find any other unix that does this. So I think it is safe to clarify > > >the definition of current process group, to current process group > > >at the time of the wait function. > > > > > >Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > > >--- > > > kernel/exit.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > >diff --git a/kernel/exit.c b/kernel/exit.c > > >index a75b6a7f458a..3d86930f035e 100644 > > >--- a/kernel/exit.c > > >+++ b/kernel/exit.c > > >@@ -1577,14 +1577,16 @@ static long kernel_waitid(int which, pid_t > > >upid, struct waitid_info *infop, > > > break; > > > case P_PGID: > > > type = PIDTYPE_PGID; > > >- if (upid <= 0) > > >+ if (upid < 0) > > > return -EINVAL; > > >+ if (upid == 0) > > >+ pid = get_pid(task_pgrp(current)); > > > break; > > > default: > > > return -EINVAL; > > > } > > > > > >- if (type < PIDTYPE_MAX) > > >+ if ((type < PIDTYPE_MAX) && !pid) > > > pid = find_get_pid(upid); > > > > > > wo.wo_type = type; > > > > Eric, mind if I send this out alongside the P_PIDFD patchset and put in a test for it? > > Was this ever sent? It's not upstream. Would be really nice to get this fixed in 5.3 so that RV32 will not be missing functionality... Rich
On August 14, 2019 1:11:53 AM GMT+02:00, Rich Felker <dalias@libc.org> wrote: >On Tue, Aug 13, 2019 at 03:22:02PM -0700, Alistair Francis wrote: >> On Thu, Jul 25, 2019 at 10:30 AM Christian Brauner ><christian@brauner.io> wrote: >> > >> > On July 25, 2019 7:14:05 PM GMT+02:00, ebiederm@xmission.com wrote: >> > >Arnd Bergmann <arnd@arndb.de> writes: >> > > >> > >> On Thu, Jul 25, 2019 at 6:40 AM Rich Felker <dalias@libc.org> >wrote: >> > >>> On Wed, Jul 24, 2019 at 05:04:53PM -0700, Alistair Francis >wrote: >> > >>> > On Tue, Jul 23, 2019 at 1:45 AM Arnd Bergmann <arnd@arndb.de> >> > >wrote: >> > >>> > > >> > >>> > > Sounds good to me, the debate over what rusage to use >should not >> > >hold >> > >>> > > up the review of the rest of that syscall. >> > >>> > >> > >>> > I'm unclear what the final decision is here. What is the >solution >> > >are >> > >>> > we going to have wait4() or add P_PROCESS_PGID to waitid()? >> > >>> > >> > >>> > As well as that what is the solution to current >implementations? >> > >If we >> > >>> > add wait4() then there isn't an issue (and I can drop this >patch) >> > >but >> > >>> > if we add P_PROCESS_PGID then we will need a way to handle >kernels >> > >>> > with waitid() but no P_PROCESS_PGID. Although my new plan is >to >> > >only >> > >>> > use the waitid syscall if we don't have waitpid or wait4 so >it >> > >seems >> > >>> > like this will only affect RV32 for the time being. >> > >>> >> > >>> I would really like some indication which solution will be >taken, >> > >>> since it impacts choices that will need to be made in musl very >> > >soon. >> > >>> My favorite outcome would be bringing back wait4 for rv32 (and >> > >>> no-time32 archs in general) *and* adding P_PROCESS_PGID. In the >> > >short >> > >>> term, just using wait4 would be the simplest and cleanest for >us >> > >(same >> > >>> as all other archs, no extra case to deal with), but in the >long >> > >term >> > >>> there may be value in having rusage that can represent more >than 68 >> > >>> cpu-years spent by a process (seems plausible with large >numbers of >> > >>> cores). >> > >> >> > >> Based on the feedback from Linus and Eric, the most likely >outcome >> > >> at the moment seems to be an extension of waitid() to allow >> > >> P_PGID with id=0 like BSD does, and not bring back wait4() or >> > >> add P_PROCESS_PGID. >> > >> >> > >> So far, I don't think anyone has proposed an actual kernel >patch. >> > >> I was hoping that Eric would do it, but I could also send it if >he's >> > >> otherwise busy. >> > > >> > >So here is what I am looking at. It still needs to be tested >> > >and the description needs to be improved so that it properly >credits >> > >everyone. However I think I have the major stroeks correct. >> > > >> > >From: "Eric W. Biederman" <ebiederm@xmission.com> >> > >Date: Tue, 23 Jul 2019 07:44:46 -0500 >> > >Subject: [PATCH] waitid: Add support for waiting for the current >> > >process group >> > > >> > >It was recently discovered that the linux version of waitid is not >a >> > >superset of the other wait functions because it does not include >> > >support for waiting for the current process group. This has two >> > >downsides. An extra system call is needed to get the current >process >> > >group, and a signal could come in between the system call that >> > >retrieved the process gorup and the call to waitid that changes >the >> > >current process group. >> > > >> > >Allow userspace to avoid both of those issues by defining >> > >idtype == P_PGID and id == 0 to mean wait for the caller's process >> > >group at the time of the call. >> > > >> > >Arguments can be made for using a different choice of idtype and >id >> > >for this case but the BSDs already use this P_PGID and 0 to >indicate >> > >waiting for the current process's process group. So be nice to >user >> > >space programmers and don't introduce an unnecessary >incompatibility. >> > > >> > >Some people have noted that the posix description is that >> > >waitpid will wait for the current process group, and that in >> > >the presence of pthreads that process group can change. To get >> > >clarity on this issue I looked at XNU, FreeBSD, and Luminos. All >of >> > >those flavors of unix waited for the current process group at the >> > >time of call and as written could not adapt to the process group >> > >changing after the call. >> > > >> > >At one point Linux did adapt to the current process group changing >but >> > >that stopped in 161550d74c07 ("pid: sys_wait... fixes"). It has >been >> > >over 11 years since Linux has that behavior, no programs that fail >> > >with the change in behavior have been reported, and I could not >> > >find any other unix that does this. So I think it is safe to >clarify >> > >the definition of current process group, to current process group >> > >at the time of the wait function. >> > > >> > >Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> > >--- >> > > kernel/exit.c | 6 ++++-- >> > > 1 file changed, 4 insertions(+), 2 deletions(-) >> > > >> > >diff --git a/kernel/exit.c b/kernel/exit.c >> > >index a75b6a7f458a..3d86930f035e 100644 >> > >--- a/kernel/exit.c >> > >+++ b/kernel/exit.c >> > >@@ -1577,14 +1577,16 @@ static long kernel_waitid(int which, pid_t >> > >upid, struct waitid_info *infop, >> > > break; >> > > case P_PGID: >> > > type = PIDTYPE_PGID; >> > >- if (upid <= 0) >> > >+ if (upid < 0) >> > > return -EINVAL; >> > >+ if (upid == 0) >> > >+ pid = get_pid(task_pgrp(current)); >> > > break; >> > > default: >> > > return -EINVAL; >> > > } >> > > >> > >- if (type < PIDTYPE_MAX) >> > >+ if ((type < PIDTYPE_MAX) && !pid) >> > > pid = find_get_pid(upid); >> > > >> > > wo.wo_type = type; >> > >> > Eric, mind if I send this out alongside the P_PIDFD patchset and >put in a test for it? >> >> Was this ever sent? > >It's not upstream. Would be really nice to get this fixed in 5.3 so >that RV32 will not be missing functionality... > >Rich I'll pick it up as is and send out for review later if I hear no objections. Christian
From: Christian Brauner <christian.brauner@ubuntu.com>
Hey everyone,
This patch adds support for waiting on the current process group by
specifying waitid(P_PGID, 0, ...) as discussed in [1]. The details why
we need to do this are in the commit message of [PATCH 1/1] so I won't
repeat them here.
I've picked this up since the thread has gone stale and parts of
userspace are actually blocked by this.
Note that the patch has been marked with v1 because I've changed the
patch to be more closely aligned with the P_PIDFD changes to waitid() I
have sitting in my for-next branch (cf. [2]).
This makes the merge conflict a little simpler and picks up on the
coding style discussions that guided the P_PIDFD patchset.
There was some desire to get this feature in with 5.3 (cf. [3]).
But given that this is a new feature for waitid() and for the sake of
avoiding any merge conflicts I would prefer to land this in the 5.4
merge window together with the P_PIDFD changes.
Thanks!
Christian
/* References */
[1]: https://www.sourceware.org/ml/libc-alpha/2019-07/msg00587.html
[2]: https://lore.kernel.org/lkml/20190727222229.6516-1-christian@brauner.io/
[3]: https://www.sourceware.org/ml/libc-alpha/2019-08/msg00304.html
Eric W. Biederman (1):
waitid: Add support for waiting for the current process group
kernel/exit.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
Hey everyone, This patch adds support for waiting on the current process group by specifying waitid(P_PGID, 0, ...) as discussed in [1]. The details why we need to do this are in the commit message of [PATCH 1/1] so I won't repeat them here. I've picked this up since the thread has gone stale and parts of userspace are actually blocked by this. Note that the patch has been changed to be more closely aligned with the P_PIDFD changes to waitid() I have sitting in my for-next branch (cf. [2]). This makes the merge conflict a little simpler and picks up on the coding style discussions that guided the P_PIDFD patchset. There was some desire to get this feature in with 5.3 (cf. [3]). But given that this is a new feature for waitid() and for the sake of avoiding any merge conflicts I would prefer to land this in the 5.4 merge window together with the P_PIDFD changes. Thanks! Christian /* References */ [1]: https://www.sourceware.org/ml/libc-alpha/2019-07/msg00587.html [2]: https://lore.kernel.org/lkml/20190727222229.6516-1-christian@brauner.io/ [3]: https://www.sourceware.org/ml/libc-alpha/2019-08/msg00304.html Eric W. Biederman (1): waitid: Add support for waiting for the current process group kernel/exit.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
Hey everyone, This patch adds support for waiting on the current process group by specifying waitid(P_PGID, 0, ...) as discussed in [1]. The details why we need to do this are in the commit message of [PATCH 1/1] so I won't repeat them here. I've picked this up since the thread has gone stale and parts of userspace are actually blocked by this. Note that the patch has been changed to be more closely aligned with the P_PIDFD changes to waitid() I have sitting in my for-next branch (cf. [2]). This makes the merge conflict a little simpler and picks up on the coding style discussions that guided the P_PIDFD patchset. There was some desire to get this feature in with 5.3 (cf. [3]). But given that this is a new feature for waitid() and for the sake of avoiding any merge conflicts I would prefer to land this in the 5.4 merge window together with the P_PIDFD changes. Thanks! Christian /* v0 */ Link: https://www.sourceware.org/ml/libc-alpha/2019-07/msg00587.html /* v1 */ Link: https://lore.kernel.org/lkml/20190814113822.9505-1-christian.brauner@ubuntu.com/ /* v2 */ Link: https://lore.kernel.org/lkml/20190814130732.23572-1-christian.brauner@ubuntu.com/ /* References */ [1]: https://www.sourceware.org/ml/libc-alpha/2019-07/msg00587.html [2]: https://lore.kernel.org/lkml/20190727222229.6516-1-christian@brauner.io/ [3]: https://www.sourceware.org/ml/libc-alpha/2019-08/msg00304.html Eric W. Biederman (1): waitid: Add support for waiting for the current process group kernel/exit.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
On Wed, Aug 14, 2019 at 05:43:59PM +0200, Christian Brauner wrote: > Hey everyone, > > This patch adds support for waiting on the current process group by > specifying waitid(P_PGID, 0, ...) as discussed in [1]. The details why > we need to do this are in the commit message of [PATCH 1/1] so I won't > repeat them here. > > I've picked this up since the thread has gone stale and parts of > userspace are actually blocked by this. > > Note that the patch has been changed to be more closely aligned with the > P_PIDFD changes to waitid() I have sitting in my for-next branch (cf. [2]). > This makes the merge conflict a little simpler and picks up on the > coding style discussions that guided the P_PIDFD patchset. > > There was some desire to get this feature in with 5.3 (cf. [3]). > But given that this is a new feature for waitid() and for the sake of > avoiding any merge conflicts I would prefer to land this in the 5.4 > merge window together with the P_PIDFD changes. That makes 5.4 (or later, depending on other stuff) the hard minimum for RV32 ABI. Is that acceptable? I was under the impression (perhaps mistaken) that 5.3 was going to be next LTS series which is why I'd like to have the necessary syscalls for a complete working RV32 userspace in it. If I'm wrong about that please ignore me. :-) Rich
On Wed, Aug 14, 2019 at 11:58:22AM -0400, Rich Felker wrote: > On Wed, Aug 14, 2019 at 05:43:59PM +0200, Christian Brauner wrote: > > Hey everyone, > > > > This patch adds support for waiting on the current process group by > > specifying waitid(P_PGID, 0, ...) as discussed in [1]. The details why > > we need to do this are in the commit message of [PATCH 1/1] so I won't > > repeat them here. > > > > I've picked this up since the thread has gone stale and parts of > > userspace are actually blocked by this. > > > > Note that the patch has been changed to be more closely aligned with the > > P_PIDFD changes to waitid() I have sitting in my for-next branch (cf. [2]). > > This makes the merge conflict a little simpler and picks up on the > > coding style discussions that guided the P_PIDFD patchset. > > > > There was some desire to get this feature in with 5.3 (cf. [3]). > > But given that this is a new feature for waitid() and for the sake of > > avoiding any merge conflicts I would prefer to land this in the 5.4 > > merge window together with the P_PIDFD changes. > > That makes 5.4 (or later, depending on other stuff) the hard minimum > for RV32 ABI. Is that acceptable? I was under the impression (perhaps > mistaken) that 5.3 was going to be next LTS series which is why I'd > like to have the necessary syscalls for a complete working RV32 > userspace in it. If I'm wrong about that please ignore me. :-) 5.3 is not going to be an LTS and we don't do new features after the merge window is closed anyway. :) Christian
diff --git a/kernel/exit.c b/kernel/exit.c index a75b6a7f458a..3d86930f035e 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -1577,14 +1577,16 @@ static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop, break; case P_PGID: type = PIDTYPE_PGID; - if (upid <= 0) + if (upid < 0) return -EINVAL; + if (upid == 0) + pid = get_pid(task_pgrp(current)); break; default: return -EINVAL; } - if (type < PIDTYPE_MAX) + if ((type < PIDTYPE_MAX) && !pid) pid = find_get_pid(upid); wo.wo_type = type;