Re: [PATCH v1 1/2] pid: add pidfd_open()
Hi Christian,
For next revision, could you also CC sur...@google.com as well? He is also
working on the low memory killer. And also suggest CC to
kernel-t...@android.com. And mentioned some comments below, thanks.
On Thu, May 16, 2019 at 03:59:42PM +0200, Christian Brauner wrote:
[snip]
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 20881598bdfa..4afca3d6dcb8 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -38,6 +38,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -451,6 +452,55 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> return idr_get_next(>idr, );
> }
>
> +/**
> + * pidfd_open() - Open new pid file descriptor.
> + *
> + * @pid: pid for which to retrieve a pidfd
> + * @flags: flags to pass
> + *
> + * This creates a new pid file descriptor with the O_CLOEXEC flag set for
> + * the process identified by @pid. Currently, the process identified by
> + * @pid must be 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, a cloexec pidfd is returned.
> + * On error, a negative errno number will be returned.
> + */
> +SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> +{
> + int fd, ret;
> + struct pid *p;
> + struct task_struct *tsk;
> +
> + if (flags)
> + return -EINVAL;
> +
> + if (pid <= 0)
> + return -EINVAL;
> +
> + p = find_get_pid(pid);
> + if (!p)
> + return -ESRCH;
> +
> + ret = 0;
> + rcu_read_lock();
> + /*
> + * If this returns non-NULL the pid was used as a thread-group
> + * leader. Note, we race with exec here: If it changes the
> + * thread-group leader we might return the old leader.
> + */
> + tsk = pid_task(p, PIDTYPE_TGID);
Just trying to understand the comment here. The issue is that we might either
return the new leader, or the old leader depending on the overlap with
concurrent de_thread right? In either case, we don't care though.
I suggest to remove the "Note..." part of the comment since it doesn't seem the
race is relevant here unless we are doing something else with tsk in the
function, but if you want to keep it that's also fine. Comment text should
probably should be 'return the new leader' though.
> + if (!tsk)
> + ret = -ESRCH;
Perhaps -EINVAL? AFAICS, this can only happen if a CLONE_THREAD pid was
passed as argument to pidfd_open which is invalid. But let me know what you
had in mind..
thanks,
- Joel
> + rcu_read_unlock();
> +
> + fd = ret ?: pidfd_create(p);
> + put_pid(p);
> + return fd;
> +}
> +
> void __init pid_idr_init(void)
> {
> /* Verify no one has done anything silly: */
> --
> 2.21.0
>
Re: [PATCH v2 2/2] mm: speed up mremap by 500x on large regions
On Fri, Oct 12, 2018 at 07:25:08PM -0700, Daniel Colascione wrote: [...] > > But anyway, I think this runtime detection thing is not needed. THP is > > actually expected to be as fast as this anyway, so if that's available then > > we should already be as fast. > > Ah, I think the commit message is confusing. (Or else I'm misreading > the patch now.) It's not quite that we're disabling the feature when > THP is enabled anywhere, but rather that we use the move_huge_pmd path > for huge PMDs and use the new code only for non-huge PMDs. (Right?) If > that's the case, the commit message shouldn't say "Incase THP is > enabled, the optimization is skipped". Even if THP is enabled on a > system generally, we might use the new PMD-moving code for mapping > types that don't support THP-ization, right? That is true. Ok, I guess I can update the commit message to be more accurate about that. > > This is for non-THP where THP cannot be enabled > > and there is still room for some improvement. Most/all architectures will be > > just fine with this. This flag is more of a safety-net type of thing where > > in > > the future if there is this one or two weird architectures that don't play > > well, then they can turn it off at the architecture level by not selecting > > the flag. See my latest patches for the per-architecture compile-time > > controls. Ideally we'd like to blanket turn it on on all, but this is just > > playing it extra safe as Kirill and me were discussing on other threads. > > Sure. I'm just pointing out that the 500x performance different turns > the operation into a qualitatively different feature, so if we expect > to actually ship a mainstream architecture without support for this > thing, we should make it explicit. If we're not, we shouldn't. We can make it explicit by enabling it in such a mainstream architecture is my point. Also if the optimization is not doing what its supposed to, then userspace will also just know by measuring the time. thanks, - Joel
Re: [PATCH v2 2/2] mm: speed up mremap by 500x on large regions
On Fri, Oct 12, 2018 at 06:54:33PM -0700, Daniel Colascione wrote: > I wonder whether it makes sense to expose to userspace somehow whether > mremap is "fast" for a particular architecture. If a feature relies on > fast mremap, it might be better for some userland component to disable > that feature entirely rather than blindly use mremap and end up > performing very poorly. If we're disabling fast mremap when THP is > enabled, the userland component can't just rely on an architecture > switch and some kind of runtime feature detection becomes even more > important. I hate to point out that its forbidden to top post on LKML :-) https://kernelnewbies.org/mailinglistguidelines So don't that Mr. Dan! :D But anyway, I think this runtime detection thing is not needed. THP is actually expected to be as fast as this anyway, so if that's available then we should already be as fast. This is for non-THP where THP cannot be enabled and there is still room for some improvement. Most/all architectures will be just fine with this. This flag is more of a safety-net type of thing where in the future if there is this one or two weird architectures that don't play well, then they can turn it off at the architecture level by not selecting the flag. See my latest patches for the per-architecture compile-time controls. Ideally we'd like to blanket turn it on on all, but this is just playing it extra safe as Kirill and me were discussing on other threads. thanks! - Joel
Re: [PATCH v2 2/2] mm: speed up mremap by 500x on large regions
On Fri, Oct 12, 2018 at 06:39:45PM -0700, Daniel Colascione wrote: > Not 32-bit ARM? Well, I didn't want to enable every possible architecture we could in a single go. Certainly arm32 can be a follow on enablement as can be other architectures. The point of this series is to upstream this feature and enable a hand-picked few architectures as a first step. thanks, - Joel
Re: [PATCH v2 2/2] mm: speed up mremap by 500x on large regions
On Fri, Oct 12, 2018 at 11:18:36AM -0700, David Miller wrote: > From: Joel Fernandes [...] > > Also, do we not flush the caches from any path when we munmap > > address space? We do call do_munmap on the old mapping from mremap > > after moving to the new one. > > Sparc makes sure that shared mapping have consistent colors. Therefore > all that's left are private mappings and those will be initialized by > block stores to clear the page out or similar. > > Also, when creating new mappings, we flush the D-cache when necessary > in update_mmu_cache(). > > We also maintain a bit in the page struct to track when a page which > was potentially written to on one cpu ends up mapped into another > address space and flush as necessary. > > The cache is write-through, which simplifies the preconditions we have > to maintain. Makes sense, thanks. For the moment I sent patches to enable this on arm64 and x86. We can enable it on sparc as well at a later time as it sounds it could be a safe optimization to apply to that architecture as well. thanks, - Joel
