Messages in this thread | | | From | Linus Torvalds <> | Date | Sat, 27 Jul 2019 09:28:40 -0700 | Subject | Re: [PATCH v2 1/2] pidfd: add P_PIDFD to waitid() |
| |
Sorry to keep pestering about the patch series, but with the addition of P_PIDFD, I react once again..
On Sat, Jul 27, 2019 at 1:53 AM Christian Brauner <christian@brauner.io> wrote: > > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -1555,6 +1555,7 @@ static long do_wait(struct wait_opts *wo) > static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop, > int options, struct rusage *ru) > { > + struct fd f;
Please don't do 'struct fd' at this level. That results in this ugly code later:
> - put_pid(pid); > + if (which == P_PIDFD) > + fdput(f); > + else > + put_pid(pid);
which just looks nasty.
Instead, do all the 'file descriptor to pid' games here:
> + case P_PIDFD: > + type = PIDTYPE_PID; > + if (upid < 0) > + return -EINVAL; > + > + f = fdget(upid); > + if (!f.file) > + return -EBADF; > + > + pid = pidfd_pid(f.file); > + if (IS_ERR(pid)) { > + fdput(f); > + return PTR_ERR(pid); > + } > break;
and make thus just do something like
pid = get_pid_from_fd(upid); if (IS_ERR(pid)) return PTR_ERR(pid);
and now do that "fd to pid" in that helper function, and get the reference to 'struct pid *' there instead.
Which you can actually do efficiently and lightly without even getting a ref to the 'struct file'. Something like
struct pid *fd_to_pid(unsigned int fd) { struct fd f; struct pid *pid;
f = fdget(fd); if (!f.file) return ERR_PTR(-EBADF); pid = pidfd_pid(f.file); if (!IS_ERR(pid)) get_pid(pid); fdput(f); return pid; }
is the stupid and straightforward thing, but if you want to be *clever* you can actually avoid getting a reference to the 'struct file *" entirely, and do the fd->pid lookup under rcu_read_lock() instead. It's slightly more complex, but it avoids the fdget/fdput reference count games entirely.
And then all that kernel_waitid() ever worries about is "struct pid *", and the ending goes back to just that simple
put_pid(pid); return ret;
instead.
This was kind of my point of doing all the "find_get_pid()" games in the "switch()" statement - the different cases have different ways to look up what the "struct pid *" pointer should be, but they should all just look up a pid pointer, and then nothing else needs to care about 'type' any more. See?
Hmm?
Linus
| |