From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 97243C433F5 for ; Mon, 11 Oct 2021 10:45:31 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BD76960E9C for ; Mon, 11 Oct 2021 10:45:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org BD76960E9C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ubuntu.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 4AE2A6B006C; Mon, 11 Oct 2021 06:45:30 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 45E456B0071; Mon, 11 Oct 2021 06:45:30 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 34D3D900002; Mon, 11 Oct 2021 06:45:30 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0144.hostedemail.com [216.40.44.144]) by kanga.kvack.org (Postfix) with ESMTP id 236D86B006C for ; Mon, 11 Oct 2021 06:45:30 -0400 (EDT) Received: from smtpin34.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id DBF09180938FE for ; Mon, 11 Oct 2021 10:45:29 +0000 (UTC) X-FDA: 78683825178.34.DB8E9CE Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf08.hostedemail.com (Postfix) with ESMTP id 589B53003183 for ; Mon, 11 Oct 2021 10:45:29 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id 020F16023D; Mon, 11 Oct 2021 10:45:22 +0000 (UTC) Date: Mon, 11 Oct 2021 12:45:15 +0200 From: Christian Brauner To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Vlastimil Babka , Suren Baghdasaryan , Matthew Bobrowski , Alexander Duyck , Jan Kara , Minchan Kim Subject: Re: [PATCH 1/2] pid: add pidfd_get_task() helper Message-ID: <20211011104515.exvbxumlxhw7deij@wittgenstein> References: <20211004125050.1153693-1-christian.brauner@ubuntu.com> <20211004125050.1153693-2-christian.brauner@ubuntu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 589B53003183 X-Stat-Signature: zgnuzg7j4sfs1zi75u6e1rf4t5t1z3oc Authentication-Results: imf08.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf08.hostedemail.com: domain of "SRS0=Y9yr=O7=ubuntu.com=christian.brauner@kernel.org" designates 198.145.29.99 as permitted sender) smtp.mailfrom="SRS0=Y9yr=O7=ubuntu.com=christian.brauner@kernel.org" X-HE-Tag: 1633949129-647022 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Oct 08, 2021 at 10:47:36AM +0200, David Hildenbrand wrote: > On 04.10.21 14:50, Christian Brauner wrote: > > The number of system calls making use of pidfds is constantly > > increasing. Some of those new system calls duplicate the code to turn a > > pidfd into task_struct it refers to. Give them a simple helper for this. > > > > Cc: Vlastimil Babka > > Cc: Suren Baghdasaryan > > Cc: Matthew Bobrowski > > Cc: Alexander Duyck > > Cc: Jan Kara > > Cc: Minchan Kim > > Signed-off-by: Christian Brauner > > --- > > include/linux/pid.h | 1 + > > kernel/pid.c | 34 ++++++++++++++++++++++++++++++++++ > > 2 files changed, 35 insertions(+) > > > > diff --git a/include/linux/pid.h b/include/linux/pid.h > > index af308e15f174..343abf22092e 100644 > > --- a/include/linux/pid.h > > +++ b/include/linux/pid.h > > @@ -78,6 +78,7 @@ struct file; > > extern struct pid *pidfd_pid(const struct file *file); > > struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); > > +struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags); > > int pidfd_create(struct pid *pid, unsigned int flags); > > static inline struct pid *get_pid(struct pid *pid) > > diff --git a/kernel/pid.c b/kernel/pid.c > > index efe87db44683..2ffbb87b2ce8 100644 > > --- a/kernel/pid.c > > +++ b/kernel/pid.c > > @@ -539,6 +539,40 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) > > return pid; > > } > > +/** > > + * pidfd_get_task() - Get the task associated with a pidfd > > + * > > + * @pidfd: pidfd for which to get the task > > + * @flags: flags associated with this pidfd > > + * > > + * Return the task associated with the given pidfd. > > + * Currently, the process identified by @pidfd is always a thread-group leader. > > + * This restriction currently exists for all aspects of pidfds including pidfd > > + * creation (CLONE_PIDFD cannot be used with CLONE_THREAD) and pidfd polling > > + * (only supports thread group leaders). > > + * > > + * Return: On success, the task_struct associated with the pidfd. > > + * On error, a negative errno number will be returned. > > Nice doc. > > You might want to document what callers of this function are expected to do > to clean up. That's a good idea! Let me add that. > > > > + */ > > +struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags) > > +{ > > + unsigned int f_flags; > > + struct pid *pid; > > + struct task_struct *task; > > + > > + pid = pidfd_get_pid(pidfd, &f_flags); > > + if (IS_ERR(pid)) > > + return ERR_CAST(pid); > > + > > + task = get_pid_task(pid, PIDTYPE_TGID); > > + put_pid(pid); > > The code to be replaced always does the put_pid() after the > put_task_struct(). Is this new ordering safe? (didn't check) I at least see no obvious problems and so do think this is safe. The lifetimes of struct pid and struct task_struct are independent of each other. They don't mess with each others refcounts. And the caller's aren't going back from struct task_struct to struct pid anywhere. > > > + if (!task) > > + return ERR_PTR(-ESRCH); > > + > > + *flags = f_flags; > > + return task; > > +} > > + > > /** > > * pidfd_create() - Create a new pid file descriptor. > > * > > > > I'd have squashed this into the second patch, makes it a lot easier to > review and it's only a MM cleanup at this point. Hm, I prefer the split between introducing a helper and making use of a helper. I find that nicer than mixing up the two steps. I only tend to do both if it would introduce breakage.