Re: [PATCH RFC 1/2] Add polling support to pidfd

From: Daniel Colascione <dancol@google.com>
To: Jonathan Kowalski <bl0pbl33p@gmail.com>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	Oleg Nesterov <oleg@redhat.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Steven Rostedt <rostedt@goodmis.org>,
	Christian Brauner <christian@brauner.io>,
	Jann Horn <jannh@google.com>,
	Suren Baghdasaryan <surenb@google.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Andrei Vagin <avagin@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Kees Cook <keescook@chromium.org>,
	Linux FS Devel <linux-fsdevel@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>, Michal Hocko <mhocko@suse.com>,
	Nadav Amit <namit@vmware.com>, Serge Hallyn <serge@hallyn.com>,
	Shuah Khan <shuah@kernel.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>,
	Taehee Yoo <ap420073@gmail.com>, Tejun Heo <tj@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Android Kernel Team <kernel-team@android.com>,
	Tycho Andersen <tycho@tycho.ws>
Subject: Re: [PATCH RFC 1/2] Add polling support to pidfd
Date: Thu, 18 Apr 2019 11:57:54 -0700	[thread overview]
Message-ID: <CAKOZueu=5aNf+oDFBM56TbqZwKpB6q1y81V1cbL+k4DTyV53+A@mail.gmail.com> (raw)
In-Reply-To: <CAGLj2rELsr0PUgu41uZX_TKCpT-Q00X+cvgBhJ-U7RU3EBo5tA@mail.gmail.com>

On Thu, Apr 18, 2019 at 11:44 AM Jonathan Kowalski <bl0pbl33p@gmail.com> wrote:
>
> On Tue, Apr 16, 2019 at 8:21 PM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Tue, Apr 16, 2019 at 02:04:31PM +0200, Oleg Nesterov wrote:
> > > On 04/11, Joel Fernandes (Google) wrote:
> > > >
> > > > +static unsigned int proc_tgid_base_poll(struct file *file, struct poll_table_struct *pts)
> > > > +{
> > > > +   int poll_flags = 0;
> > > > +   struct task_struct *task;
> > > > +   struct pid *pid;
> > > > +
> > > > +   task = get_proc_task(file->f_path.dentry->d_inode);
> > > > +
> > > > +   WARN_ON_ONCE(task && !thread_group_leader(task));
> > > > +
> > > > +   /*
> > > > +    * tasklist_lock must be held because to avoid racing with
> > > > +    * changes in exit_state and wake up. Basically to avoid:
> > > > +    *
> > > > +    * P0: read exit_state = 0
> > > > +    * P1: write exit_state = EXIT_DEAD
> > > > +    * P1: Do a wake up - wq is empty, so do nothing
> > > > +    * P0: Queue for polling - wait forever.
> > > > +    */
> > > > +   read_lock(&tasklist_lock);
> > > > +   if (!task)
> > > > +           poll_flags = POLLIN | POLLRDNORM | POLLERR;
> > > > +   else if (task->exit_state == EXIT_DEAD)
> > > > +           poll_flags = POLLIN | POLLRDNORM;
> > > > +   else if (task->exit_state == EXIT_ZOMBIE && thread_group_empty(task))
> > > > +           poll_flags = POLLIN | POLLRDNORM;
> > > > +
> > > > +   if (!poll_flags) {
> > > > +           pid = proc_pid(file->f_path.dentry->d_inode);
> > > > +           poll_wait(file, &pid->wait_pidfd, pts);
> > > > +   }
> > >
> > > can't understand...
> > >
> > > Could you explain when it should return POLLIN? When the whole process exits?
> >
> > It returns POLLIN when the task is dead or doesn't exist anymore, or when it
> > is in a zombie state and there's no other thread in the thread group.
> >
>
> Would using something other than POLLIN be an option (maybe POLLPRI)?
> The convention is to use it to indicate readability on the descriptor,
> and also possibly POLLHUP instead of POLLERR (the latter is less of a
> problem, but FreeBSD also does the same, so it'd help with some
> consistency for libraries wanting to use this, which aren't interested
> in other sub states).

Existing event loop libraries generally support checking only for
readability and writability. Not setting POLLIN would make these FDs
more difficult to use with existing event loop libraries. What
advantage would compensate for this difficulty?