* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Christopher Lameter @ 2018-05-03 18:03 UTC (permalink / raw)
To: Prakash Sangappa
Cc: linux-kernel, linux-mm, linux-api, akpm, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes
In-Reply-To: <1525240686-13335-1-git-send-email-prakash.sangappa@oracle.com>
On Tue, 1 May 2018, Prakash Sangappa wrote:
> For analysis purpose it is useful to have numa node information
> corresponding mapped address ranges of the process. Currently
> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
> allocated per VMA of the process. This is not useful if an user needs to
> determine which numa node the mapped pages are allocated from for a
> particular address range. It would have helped if the numa node information
> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
> exact numa node from where the pages have been allocated.
Cant you write a small script that scans the information in numa_maps and
then displays the total pages per NUMA node and then a list of which
ranges have how many pages on a particular node?
> reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
So a prime motivator here is security restricted access to numa_maps?
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Joel Fernandes @ 2018-05-03 17:46 UTC (permalink / raw)
To: Daniel Colascione
Cc: Mathieu Desnoyers, Peter Zijlstra, Paul McKenney, Boqun Feng,
Andy Lutomirski, davejwatson, LKML, linux-api, Paul Turner,
Andrew Morton, linux, Thomas Gleixner, Ingo Molnar, hpa,
Andrew Hunter, andi, cl, bmaurer, Steven Rostedt, Josh Triplett,
torvalds, Catalin Marinas, Will Deacon, mtk.manpages
In-Reply-To: <CAKOZueve1K8bmS0pmeCAAburoJtzRA4DeN=OCyW3kHiNo8WO=A@mail.gmail.com>
Hi Daniel,
Nice to have this healthy discussion about pros/cons. Adding Waiman to the
discussion as well. Curious to hear what Waiman and Peter think about all
this. Some more comments inline.
On Thu, May 3, 2018 at 10:19 AM Daniel Colascione <dancol@google.com> wrote:
> On Thu, May 3, 2018 at 9:48 AM Joel Fernandes <joelaf@google.com> wrote:
> > > > can skip the manual schedule we were going to perform.
> > > By the way, if we eventually find a way to enhance user-space mutexes
in
> > the
> > > fashion you describe here, it would belong to another TLS area, and
> would
> > > be registered by another system call than rseq. I proposed a more
> generic
> > Right. Also I still don't see any good reason why optimistic spinning in
> > the kernel with FUTEX_LOCK, as Peter described, can't be used instead of
> > using the rseq implementation and spinning in userspace, for such a
case.
> I
> > don't really fully buy that we need to design this interface assuming
any
> > privilege transition level time.
> > If privilege level transitions are slow,
> > we're going to have bad performance anyway.
> That's not the case. There's a large class of program that does useful
work
> while seldom entering the kernel: just ask the user-space network stack
> people.
Yes, I am aware of that. I was just saying in general, a system such as an
Android embedded system, not an HPC based system does make a lot of system
calls. I am not arguing that doing more things in userspace is good or bad
here. I am just talking about why do something else for no good reasons
(see below) when work has already been done on this area.
> It's not wise to design interfaces around system calls being cheap. Even
if
> system calls are currently cheap enough on some architectures some of the
> time, there's no guarantee that they'll stay that way, especially relative
> to straight-line user-mode execution. A pure user-space approach, on the
> other hand, involves no work in the kernel, and doing nothing is always
the
> optimal strategy. Besides, there are environments where system calls end
up
> being more expensive than you might think: consider strace or rr. If the
> kernel needs to get involved on some path, it's best that its involvement
> be as light as possible.
Ofcourse, but I think we shouldn't do a premature optimization here without
real data on typical Android devices about the cost of system calls
entry/exit, vs spin time. I am not against userspace lock based on rseq if
there is data and good reason, before investing significant time on
reinventing the wheel.
> > we should really stick to using FUTEX_LOCK and
> > reuse all the work that went into that area for Android and otherwise
(and
> > work with Waiman and others on improving that if there are any problems
> > with it).
> FUTEX_LOCK is a return to the bad old days when systems gave you a fixed
> list of synchronization primitives and if you wanted something else,
tough.
I am not saying we should fix sync. primitives made available to userspace,
or anything. I am talking about yours/our usecase and whether another sync
primitive interface is needed. For example, have another syscall to
register TLS area is a new interface, vs using the existing futex
interface. Linus is also against adding new sycalls unnecessarily.
> That the latest version of the FUTEX_LOCK patch includes a separate
> FUTEX_LOCK_SHARED mode is concerning. The functionality the kernel
provides
Why? That's just for reader-locks. What's the concern there? I know you had
something in mind about efficient userspace rw locks but I am curious
either way what you have in mind.
> to userspace should be more general-purpose and allow more experimentation
> without changes in the kernel. I see no reason to force userspace into 1)
> reserving 30 bits of its lockword for a TID and 2) adopting the kernel's
Based on our offline chat, this is for only 32-bit only systems though
right? Also based on Peter's idea of putting the recursion counter outside,
there shouldn't be a space issue?
> idea of spin time heuristics and lock stealing when the same basic
> functionality can be provided in a generic way while reserving only one
> bit. That this mechanism happens to be more efficient as well is a bonus.
And also probably easy to get wrong. Heuristics are hard and it would be
good to work with community on getting best approach for that and improving
existing code. Also about "generic way", that's even more reason in my view
to do it in the kernel.
> "Mechanism not policy" is still a good design principle.
Again, I am not advocating forcing of interfaces anything, but I'm against
reinventing the wheel and am all for spending time on improving existing
things.
thanks!
- Joel
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Daniel Colascione @ 2018-05-03 17:18 UTC (permalink / raw)
To: Joel Fernandes
Cc: Mathieu Desnoyers, Peter Zijlstra, Paul McKenney, boqun.feng,
luto, davejwatson, linux-kernel, linux-api, Paul Turner,
Andrew Morton, linux, tglx, mingo, hpa, Andrew Hunter, andi, cl,
bmaurer, rostedt, josh, torvalds, catalin.marinas, will.deacon,
Michael Kerrisk-manpages
In-Reply-To: <CAJWu+opXjYrjrz1=k0TjjCqiYQ0y=6jRMiRWNvb8_dHAtSByug@mail.gmail.com>
On Thu, May 3, 2018 at 9:48 AM Joel Fernandes <joelaf@google.com> wrote:
> > > can skip the manual schedule we were going to perform.
> > By the way, if we eventually find a way to enhance user-space mutexes in
> the
> > fashion you describe here, it would belong to another TLS area, and
would
> > be registered by another system call than rseq. I proposed a more
generic
> Right. Also I still don't see any good reason why optimistic spinning in
> the kernel with FUTEX_LOCK, as Peter described, can't be used instead of
> using the rseq implementation and spinning in userspace, for such a case.
I
> don't really fully buy that we need to design this interface assuming any
> privilege transition level time.
> If privilege level transitions are slow,
> we're going to have bad performance anyway.
That's not the case. There's a large class of program that does useful work
while seldom entering the kernel: just ask the user-space network stack
people.
It's not wise to design interfaces around system calls being cheap. Even if
system calls are currently cheap enough on some architectures some of the
time, there's no guarantee that they'll stay that way, especially relative
to straight-line user-mode execution. A pure user-space approach, on the
other hand, involves no work in the kernel, and doing nothing is always the
optimal strategy. Besides, there are environments where system calls end up
being more expensive than you might think: consider strace or rr. If the
kernel needs to get involved on some path, it's best that its involvement
be as light as possible.
> we should really stick to using FUTEX_LOCK and
> reuse all the work that went into that area for Android and otherwise (and
> work with Waiman and others on improving that if there are any problems
> with it).
FUTEX_LOCK is a return to the bad old days when systems gave you a fixed
list of synchronization primitives and if you wanted something else, tough.
That the latest version of the FUTEX_LOCK patch includes a separate
FUTEX_LOCK_SHARED mode is concerning. The functionality the kernel provides
to userspace should be more general-purpose and allow more experimentation
without changes in the kernel. I see no reason to force userspace into 1)
reserving 30 bits of its lockword for a TID and 2) adopting the kernel's
idea of spin time heuristics and lock stealing when the same basic
functionality can be provided in a generic way while reserving only one
bit. That this mechanism happens to be more efficient as well is a bonus.
"Mechanism not policy" is still a good design principle.
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Joel Fernandes @ 2018-05-03 16:48 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Daniel Colascione, Peter Zijlstra, Paul McKenney, Boqun Feng,
Andy Lutomirski, davejwatson, LKML, linux-api, Paul Turner,
Andrew Morton, linux, Thomas Gleixner, Ingo Molnar, hpa,
Andrew Hunter, andi, cl, bmaurer, Steven Rostedt, Josh Triplett,
torvalds, Catalin Marinas, Will Deacon, mtk.manpages
In-Reply-To: <1718748931.10084.1525363941807.JavaMail.zimbra@efficios.com>
On Thu, May 3, 2018 at 9:12 AM Mathieu Desnoyers <
mathieu.desnoyers@efficios.com> wrote:
> ----- On May 2, 2018, at 12:07 PM, Daniel Colascione dancol@google.com
wrote:
> > On Wed, May 2, 2018 at 9:03 AM Mathieu Desnoyers <
> > mathieu.desnoyers@efficios.com> wrote:
> >
> >> ----- On May 1, 2018, at 11:53 PM, Daniel Colascione dancol@google.com
> > wrote:
> >> [...]
> >> >
> >> > I think a small enhancement to rseq would let us build a perfect
> > userspace
> >> > mutex, one that spins on lock-acquire only when the lock owner is
> > running
> >> > and that sleeps otherwise, freeing userspace from both specifying
ad-hoc
> >> > spin counts and from trying to detect situations in which spinning is
> >> > generally pointless.
> >> >
> >> > It'd work like this: in the per-thread rseq data structure, we'd
> > include a
> >> > description of a futex operation for the kernel would perform (in the
> >> > context of the preempted thread) upon preemption, immediately before
> >> > schedule(). If the futex operation itself sleeps, that's no problem:
we
> >> > will have still accomplished our goal of running some other thread
> > instead
> >> > of the preempted thread.
> >
> >> Hi Daniel,
> >
> >> I agree that the problem you are aiming to solve is important. Let's
see
> >> what prevents the proposed rseq implementation from doing what you
> > envision.
> >
> >> The main issue here is touching userspace immediately before
schedule().
> >> At that specific point, it's not possible to take a page fault. In the
> > proposed
> >> rseq implementation, we get away with it by raising a task struct flag,
> > and using
> >> it in a return to userspace notifier (where we can actually take a
> > fault), where
> >> we touch the userspace TLS area.
> >
> >> If we can find a way to solve this limitation, then the rest of your
> > design
> >> makes sense to me.
> >
> > Thanks for taking a look!
> >
> > Why couldn't we take a page fault just before schedule? The reason we
can't
> > take a page fault in atomic context is that doing so might call
schedule.
> > Here, we're about to call schedule _anyway_, so what harm does it do to
> > call something that might call schedule? If we schedule via that call,
we
> > can skip the manual schedule we were going to perform.
> By the way, if we eventually find a way to enhance user-space mutexes in
the
> fashion you describe here, it would belong to another TLS area, and would
> be registered by another system call than rseq. I proposed a more generic
Right. Also I still don't see any good reason why optimistic spinning in
the kernel with FUTEX_LOCK, as Peter described, can't be used instead of
using the rseq implementation and spinning in userspace, for such a case. I
don't really fully buy that we need to design this interface assuming any
privilege transition level time. If privilege level transitions are slow,
we're going to have bad performance anyway. Unless there's some data to
show that we have to optimistically spin in userspace than the kernel
because its better to do so, we should really stick to using FUTEX_LOCK and
reuse all the work that went into that area for Android and otherwise (and
work with Waiman and others on improving that if there are any problems
with it).
I am excited though about the other synchronization design other than lock
implementation that rseq can help in.
thanks!
- Joel
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Daniel Colascione @ 2018-05-03 16:22 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Paul McKenney, boqun.feng, luto, davejwatson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, linux, tglx,
mingo, hpa, Andrew Hunter, andi, cl, bmaurer, rostedt, josh,
torvalds, catalin.marinas, will.deacon, Michael Kerrisk-manpages,
Joel Fernandes
In-Reply-To: <1718748931.10084.1525363941807.JavaMail.zimbra@efficios.com>
On Thu, May 3, 2018 at 9:12 AM Mathieu Desnoyers <
mathieu.desnoyers@efficios.com> wrote:
> By the way, if we eventually find a way to enhance user-space mutexes in
the
> fashion you describe here, it would belong to another TLS area, and would
> be registered by another system call than rseq. I proposed a more generic
> "TLS area registration" system call a few years ago, but Linus told me he
> wanted a system call that was specific to rseq. If we need to implement
> other use-cases in a TLS area shared between kernel and user-space in a
> similar fashion, the plan is to do it in a distinct system call.
If we proliferate TLS areas; we'd have to register each one upon thread
creation, adding to the overall thread creation path. There's already a
provision for versioning the TLS area. What's the benefit of splitting the
registration over multiple system calls?
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Mathieu Desnoyers @ 2018-05-03 16:12 UTC (permalink / raw)
To: Daniel Colascione
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Linus Torvalds, Catalin Marinas <cata>
In-Reply-To: <CAKOZueu_7Ha_WXMyxqMEScXo1aHWr9qYRxqyb-Rpd4k1JP3xHA@mail.gmail.com>
----- On May 2, 2018, at 12:07 PM, Daniel Colascione dancol@google.com wrote:
> On Wed, May 2, 2018 at 9:03 AM Mathieu Desnoyers <
> mathieu.desnoyers@efficios.com> wrote:
>
>> ----- On May 1, 2018, at 11:53 PM, Daniel Colascione dancol@google.com
> wrote:
>> [...]
>> >
>> > I think a small enhancement to rseq would let us build a perfect
> userspace
>> > mutex, one that spins on lock-acquire only when the lock owner is
> running
>> > and that sleeps otherwise, freeing userspace from both specifying ad-hoc
>> > spin counts and from trying to detect situations in which spinning is
>> > generally pointless.
>> >
>> > It'd work like this: in the per-thread rseq data structure, we'd
> include a
>> > description of a futex operation for the kernel would perform (in the
>> > context of the preempted thread) upon preemption, immediately before
>> > schedule(). If the futex operation itself sleeps, that's no problem: we
>> > will have still accomplished our goal of running some other thread
> instead
>> > of the preempted thread.
>
>> Hi Daniel,
>
>> I agree that the problem you are aiming to solve is important. Let's see
>> what prevents the proposed rseq implementation from doing what you
> envision.
>
>> The main issue here is touching userspace immediately before schedule().
>> At that specific point, it's not possible to take a page fault. In the
> proposed
>> rseq implementation, we get away with it by raising a task struct flag,
> and using
>> it in a return to userspace notifier (where we can actually take a
> fault), where
>> we touch the userspace TLS area.
>
>> If we can find a way to solve this limitation, then the rest of your
> design
>> makes sense to me.
>
> Thanks for taking a look!
>
> Why couldn't we take a page fault just before schedule? The reason we can't
> take a page fault in atomic context is that doing so might call schedule.
> Here, we're about to call schedule _anyway_, so what harm does it do to
> call something that might call schedule? If we schedule via that call, we
> can skip the manual schedule we were going to perform.
By the way, if we eventually find a way to enhance user-space mutexes in the
fashion you describe here, it would belong to another TLS area, and would
be registered by another system call than rseq. I proposed a more generic
"TLS area registration" system call a few years ago, but Linus told me he
wanted a system call that was specific to rseq. If we need to implement
other use-cases in a TLS area shared between kernel and user-space in a
similar fashion, the plan is to do it in a distinct system call.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 0/6 resend] statfs: handle mount propagation
From: Christian Brauner @ 2018-05-03 13:57 UTC (permalink / raw)
To: Al Viro
Cc: Christian Brauner, Linus Torvalds, linux-fsdevel,
Linux Kernel Mailing List, Christoph Hellwig, Thomas Gleixner,
Kate Stewart, Greg KH, Philippe Ombredanne, Linux API
In-Reply-To: <20180503134317.GA30522@ZenIV.linux.org.uk>
On Thu, May 03, 2018 at 02:43:17PM +0100, Al Viro wrote:
> On Thu, May 03, 2018 at 03:04:36PM +0200, Christian Brauner wrote:
>
> > >From a userspace perspective we often run into the case where we simply
> > want to know whether a given mountpoint is MS_SHARED or is MS_SLAVE.
> > If it is we remount it as MS_PRIVATE to prevent any propagation from
> > happening. We don't care about the peer relationship or how the
> > propagation is exactly setup. We only want to prevent any propagation
> > from happening.
>
> So what's to stop you from doing exactly that --
> mount(NULL, "/", NULL, MS_PRIVATE | MS_REC, NULL)
> without bothering with statfs() in the first place? It's not like the
> damn thing had been costly - it's O(mounts in your namespace) with not
> to high constant, and in any case cheaper than allocating all of them
> back when you did clone(2). Confused...
That's the case for the whole rootfs. But there are cases where I want
to leave the rootfs itself alone and only remount some paths.
>
> > The above case is what I see most often. A more specific use-case is to
> > differentiate between MS_SLAVE and MS_SHARED mountpoints.
> > Mountpoints that are MS_SLAVE are kept intact and mountpoints that are
> > MS_SHARED are made MS_PRIVATE.
> >
> > For both cases the only way to do this right now is by parsing
> > /proc/<pid>/mountinfo. Yes, it is doable but still it is somewhat costly
> > and annoying as e.g. those mount propagation fields are optional.
>
> Umm... And how would you get the list of mountpoints to feed to statfs()
> if not by parsing mountinfo? IDGI...
There are cases where you have list of known-mountpoints or paths to
check but you don't necessarily know whether they are MS_SHARED or
MS_SLAVE.
There's also a case where I send a list of fds via SCM_RIGHTS to an
isolated process that calls fstatvfs() on these fds to determine their
mount flags without changing them but cache this info.
Christian
^ permalink raw reply
* Re: [PATCH 0/6 resend] statfs: handle mount propagation
From: Al Viro @ 2018-05-03 13:43 UTC (permalink / raw)
To: Christian Brauner
Cc: Linus Torvalds, linux-fsdevel, Linux Kernel Mailing List, hch,
tglx, kstewart, Greg KH, pombredanne, Linux API
In-Reply-To: <CAPP7u0VPyWAsL_kkb4JnJHMmK=kM0m4zRjVjst1iHDd6V=t0mQ@mail.gmail.com>
On Thu, May 03, 2018 at 03:04:36PM +0200, Christian Brauner wrote:
> >From a userspace perspective we often run into the case where we simply
> want to know whether a given mountpoint is MS_SHARED or is MS_SLAVE.
> If it is we remount it as MS_PRIVATE to prevent any propagation from
> happening. We don't care about the peer relationship or how the
> propagation is exactly setup. We only want to prevent any propagation
> from happening.
So what's to stop you from doing exactly that --
mount(NULL, "/", NULL, MS_PRIVATE | MS_REC, NULL)
without bothering with statfs() in the first place? It's not like the
damn thing had been costly - it's O(mounts in your namespace) with not
to high constant, and in any case cheaper than allocating all of them
back when you did clone(2). Confused...
> The above case is what I see most often. A more specific use-case is to
> differentiate between MS_SLAVE and MS_SHARED mountpoints.
> Mountpoints that are MS_SLAVE are kept intact and mountpoints that are
> MS_SHARED are made MS_PRIVATE.
>
> For both cases the only way to do this right now is by parsing
> /proc/<pid>/mountinfo. Yes, it is doable but still it is somewhat costly
> and annoying as e.g. those mount propagation fields are optional.
Umm... And how would you get the list of mountpoints to feed to statfs()
if not by parsing mountinfo? IDGI...
^ permalink raw reply
* Re: [PATCH 0/6 resend] statfs: handle mount propagation
From: Christian Brauner @ 2018-05-03 13:04 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, linux-fsdevel, Linux Kernel Mailing List, hch,
tglx, kstewart, Greg KH, pombredanne, Linux API
In-Reply-To: <20180502160939.GU30522@ZenIV.linux.org.uk>
On Wed, May 02, 2018 at 05:09:39PM +0100, Al Viro wrote:
> On Wed, May 02, 2018 at 05:42:33PM +0200, Christian Brauner wrote:
> > Hey,
> >
> > This is the second resend of this patchset. I'm not sure whether it has
> > simply been overlooked but the number of people get_maintainer.pl was
> > rather small and seemed a little random so I added Linus and Christoph,
> > two people I know that do look at VFS stuff at least from time to time,
> > although they weren't listed by get_maintainer.pl. I hope that's ok.
> >
> > This little series
> > - unifies the definition of constants in statfs.h and fs.h
> > *Note, that Andreas has expressed doubts whether this unification is
> > useful. Please see https://lkml.org/lkml/2018/4/13/571 . I still think
> > it is but I'm happy to drop these two patches if others agree.*
> > - extends statfs to handle mount propagation. This will let userspace
> > easily query a given mountpoint for MS_UNBINDABLE, MS_SHARED,
> > MS_PRIVATE and MS_SLAVE without always having to do costly parsing of
> > /proc/<pid>/mountinfo.
> > To this end the flags:
> > - ST_UNBINDABLE
> > - ST_SHARED
> > - ST_PRIVATE
> > - ST_SLAVE
> > are added. They have the same value as their MS_* counterparts.
>
> How about some rationale for that in the first place? statfs() looks like
> a bad match for that - not to mention anything else, there's no way to
> get anything beyond "it is a peer of something", not even "do these two
> get propagation between them". What would be using that, what would the
> userland side of users look like, etc...
Ok, sorry if I wasn't detailed enough.
>From a userspace perspective we often run into the case where we simply
want to know whether a given mountpoint is MS_SHARED or is MS_SLAVE.
If it is we remount it as MS_PRIVATE to prevent any propagation from
happening. We don't care about the peer relationship or how the
propagation is exactly setup. We only want to prevent any propagation
from happening.
The above case is what I see most often. A more specific use-case is to
differentiate between MS_SLAVE and MS_SHARED mountpoints.
Mountpoints that are MS_SLAVE are kept intact and mountpoints that are
MS_SHARED are made MS_PRIVATE.
For both cases the only way to do this right now is by parsing
/proc/<pid>/mountinfo. Yes, it is doable but still it is somewhat costly
and annoying as e.g. those mount propagation fields are optional.
Another problem are scenarios where propagation matters but /proc is not
mounted.
Here are three concrete use-cases I run into frequently:
- (*buzzword alarm*) container runtimes:
They do usually clone(CLONE_NEWNS) and inherit the mount table but
want to remount specific mountpoints to prevent propagation.
Again the specifics of propagation don't matter in this case usually.
- Sharing shared mountpoints:
Sometimes it is desirable to share shared mountpoints between
namespaces.
Say, the first process sets up the shared mountpoint on the host and
all the other ones want to know "Did someone already make this rshared
or do i need to set this up?". It would be very helpful if you could
just check a mount by using the statvfs() syscall.
- I want to know whether a mountpoint is unbindable to e.g. skip it if
it is or remount it.
About how this would work what I simply expect and tested with this
patch was e.g. this:
int ret;
char *s = "/some/path";
struct statvfs sb;
ret = statvfs(s, &sb);
if (ret < 0)
return false;
if (sb.f_flag & ST_SHARED) {
ret = mount("", s, NULL, MS_SLAVE | MS_REC, NULL);
if (ret < 0)
return -1;
}
>
> And in any case linux-api should've been Cc'd. I'm not saying that this
Ah, thanks! Sorry I missed this.
> (or something similar) would be an inherently bad idea, but the question
> "why this way?" deserves a bit more than "parsing is costly"...
I hope some of the above helped to clarify this.
Thanks!
Christian
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Michal Hocko @ 2018-05-03 8:57 UTC (permalink / raw)
To: prakash.sangappa
Cc: Andrew Morton, linux-kernel, linux-mm, linux-api, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi, Dave Hansen
In-Reply-To: <cac754ee-efb9-0259-a50b-4efa11783058@oracle.com>
On Wed 02-05-18 16:43:58, prakash.sangappa wrote:
>
>
> On 05/02/2018 02:33 PM, Andrew Morton wrote:
> > On Tue, 1 May 2018 22:58:06 -0700 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
> >
> > > For analysis purpose it is useful to have numa node information
> > > corresponding mapped address ranges of the process. Currently
> > > /proc/<pid>/numa_maps provides list of numa nodes from where pages are
> > > allocated per VMA of the process. This is not useful if an user needs to
> > > determine which numa node the mapped pages are allocated from for a
> > > particular address range. It would have helped if the numa node information
> > > presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
> > > exact numa node from where the pages have been allocated.
> > >
> > > The format of /proc/<pid>/numa_maps file content is dependent on
> > > /proc/<pid>/maps file content as mentioned in the manpage. i.e one line
> > > entry for every VMA corresponding to entries in /proc/<pids>/maps file.
> > > Therefore changing the output of /proc/<pid>/numa_maps may not be possible.
> > >
> > > Hence, this patch proposes adding file /proc/<pid>/numa_vamaps which will
> > > provide proper break down of VA ranges by numa node id from where the mapped
> > > pages are allocated. For Address ranges not having any pages mapped, a '-'
> > > is printed instead of numa node id. In addition, this file will include most
> > > of the other information currently presented in /proc/<pid>/numa_maps. The
> > > additional information included is for convenience. If this is not
> > > preferred, the patch could be modified to just provide VA range to numa node
> > > information as the rest of the information is already available thru
> > > /proc/<pid>/numa_maps file.
> > >
> > > Since the VA range to numa node information does not include page's PFN,
> > > reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
> > >
> > > Here is the snippet from the new file content showing the format.
> > >
> > > 00400000-00401000 N0=1 kernelpagesize_kB=4 mapped=1 file=/tmp/hmap2
> > > 00600000-00601000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/tmp/hmap2
> > > 00601000-00602000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/tmp/hmap2
> > > 7f0215600000-7f0215800000 N0=1 kernelpagesize_kB=2048 dirty=1 file=/mnt/f1
> > > 7f0215800000-7f0215c00000 - file=/mnt/f1
> > > 7f0215c00000-7f0215e00000 N0=1 kernelpagesize_kB=2048 dirty=1 file=/mnt/f1
> > > 7f0215e00000-7f0216200000 - file=/mnt/f1
> > > ..
> > > 7f0217ecb000-7f0217f20000 N0=85 kernelpagesize_kB=4 mapped=85 mapmax=51
> > > file=/usr/lib64/libc-2.17.so
> > > 7f0217f20000-7f0217f30000 - file=/usr/lib64/libc-2.17.so
> > > 7f0217f30000-7f0217f90000 N0=96 kernelpagesize_kB=4 mapped=96 mapmax=51
> > > file=/usr/lib64/libc-2.17.so
> > > 7f0217f90000-7f0217fb0000 - file=/usr/lib64/libc-2.17.so
> > > ..
> > >
> > > The 'pmap' command can be enhanced to include an option to show numa node
> > > information which it can read from this new proc file. This will be a
> > > follow on proposal.
> > I'd like to hear rather more about the use-cases for this new
> > interface. Why do people need it, what is the end-user benefit, etc?
>
> This is mainly for debugging / performance analysis. Oracle Database
> team is looking to use this information.
But we do have an interface to query (e.g. move_pages) that your
application can use. I am really worried that the broken out per node
data can be really large (just take a large vma with interleaved policy
as an example). So is this really worth adding as a general purpose proc
interface?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Peter Zijlstra @ 2018-05-03 8:49 UTC (permalink / raw)
To: Daniel Colascione
Cc: Mathieu Desnoyers, Paul McKenney, boqun.feng, luto, davejwatson,
linux-kernel, linux-api, Paul Turner, Andrew Morton, linux, tglx,
mingo, hpa, Andrew Hunter, andi, cl, bmaurer, rostedt, josh,
torvalds, catalin.marinas, will.deacon, Michael Kerrisk-manpages,
Joel Fernandes
In-Reply-To: <CAKOZuevUq-XsT632VHU1TQQ5_rGqNROXiZAqv7S+ky=-wcQ3bQ@mail.gmail.com>
On Wed, May 02, 2018 at 08:37:13PM +0000, Daniel Colascione wrote:
> > Recursive locks are teh most horrible crap ever. And having the tid in
>
> What happened to providing mechanism, not policy?
>
> You can't wish away recursive locking. It's baked into Java and the CLR,
> and it's enshrined in POSIX. It's not going away, and there's no reason not
> to support it efficiently.
You can implement recursive locks just fine with a TID based word, just
keep the recursion counter external to the futex word. If owner==self,
increment etc..
> > the word allows things like kernel based optimistic spins and possibly
> > PI related things.
>
> Sure. A lot of people don't want PI though, or at least they want to opt
> into it. And we shouldn't require an entry into the kernel for what we can
> in principle do efficiently in userspace.
Any additional PI would certainly be opt-in, but the kernel based
spinning might make sense unconditionally.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Anshuman Khandual @ 2018-05-03 8:46 UTC (permalink / raw)
To: Dave Hansen, Andrew Morton, Prakash Sangappa
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi
In-Reply-To: <2ce01d91-5fba-b1b7-2956-c8cc1853536d@intel.com>
On 05/03/2018 03:58 AM, Dave Hansen wrote:
> On 05/02/2018 02:33 PM, Andrew Morton wrote:
>> On Tue, 1 May 2018 22:58:06 -0700 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
>>> For analysis purpose it is useful to have numa node information
>>> corresponding mapped address ranges of the process. Currently
>>> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
>>> allocated per VMA of the process. This is not useful if an user needs to
>>> determine which numa node the mapped pages are allocated from for a
>>> particular address range. It would have helped if the numa node information
>>> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
>>> exact numa node from where the pages have been allocated.
>
> I'm finding myself a little lost in figuring out what this does. Today,
> numa_maps might us that a 3-page VMA has 1 page from Node 0 and 2 pages
> from Node 1. We group *entirely* by VMA:
>
> 1000-4000 N0=1 N1=2
>
> We don't want that. We want to tell exactly where each node's memory is
> despite if they are in the same VMA, like this:
>
> 1000-2000 N1=1
> 2000-3000 N0=1
> 3000-4000 N1=1
I am kind of wondering on a big memory system how many lines of output
we might have for a large (consuming lets say 80 % of system RAM) VMA
in interleave policy. Is not that a problem ?
^ permalink raw reply
* Re: [RFC PATCH for 4.18 00/14] Restartable Sequences
From: Steven Rostedt @ 2018-05-03 1:15 UTC (permalink / raw)
To: Daniel Colascione
Cc: Peter Zijlstra, Mathieu Desnoyers, Paul McKenney, boqun.feng,
luto, davejwatson, linux-kernel, linux-api, Paul Turner,
Andrew Morton, linux, tglx, mingo, hpa, Andrew Hunter, andi, cl,
bmaurer, josh, torvalds, catalin.marinas, will.deacon,
Michael Kerrisk-manpages, Joel Fernandes, Robert Haas
In-Reply-To: <CAKOZuevUq-XsT632VHU1TQQ5_rGqNROXiZAqv7S+ky=-wcQ3bQ@mail.gmail.com>
On Wed, 02 May 2018 20:37:13 +0000
Daniel Colascione <dancol@google.com> wrote:
> On Wed, May 2, 2018 at 1:23 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > On Wed, May 02, 2018 at 06:27:22PM +0000, Daniel Colascione wrote:
> > > On Wed, May 2, 2018 at 10:22 AM Peter Zijlstra <peterz@infradead.org>
> wrote:
> > > >> On Wed, May 02, 2018 at 03:53:47AM +0000, Daniel Colascione wrote:
> > > > > Suppose we make a userspace mutex implemented with a lock word
> having
> > > three
> > > > > bits: acquired, sleep_mode, and wait_pending, with the rest of the
> word
> > > not
> > > > > being relevant at the moment.
> > >
> > > > So ideally we'd kill FUTEX_WAIT/FUTEX_WAKE for mutexes entirely, and
> go
> > > > with FUTEX_LOCK/FUTEX_UNLOCK that have the same semantics as the
> > > > existing FUTEX_LOCK_PI/FUTEX_UNLOCK_PI, namely, the word contains the
> > > > owner TID.
> > >
> > > That doesn't work if you want to use the rest of the word for something
> > > else, like a recursion count. With FUTEX_WAIT and FUTEX_WAKE, you can
> make
> > > a lock with two bits.
>
> > Recursive locks are teh most horrible crap ever. And having the tid in
>
> What happened to providing mechanism, not policy?
>
> You can't wish away recursive locking. It's baked into Java and the CLR,
> and it's enshrined in POSIX. It's not going away, and there's no reason not
> to support it efficiently.
>
> > the word allows things like kernel based optimistic spins and possibly
> > PI related things.
>
> Sure. A lot of people don't want PI though, or at least they want to opt
> into it. And we shouldn't require an entry into the kernel for what we can
> in principle do efficiently in userspace.
>
> > > > As brought up in the last time we talked about spin loops, why do we
> > > > care if the spin loop is in userspace or not? Aside from the whole PTI
> > > > thing, the syscall cost was around 150 cycle or so, while a LOCK
> CMPXCHG
> > > > is around 20 cycles. So ~7 spins gets you the cost of entry.
What about exit?
> > >
> > > That's pre-KPTI, isn't it?
>
> > Yes, and once the hardware gets sorted, we'll be there again. I don't
> > think we should design interfaces for 'broken' hardware.
>
> It would be a mistake to design interfaces under the assumption that
> everyone has fast permission level transitions.
Note, Robert Haas told me a few years ago at a plumbers conference that
postgresql implements their own user space spin locks because anything
that goes into the kernel has killed the performance. And they tried to
use futex but that still didn't beat out plain userspace locks.
-- Steve
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-05-02 23:43 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi, Dave Hansen
In-Reply-To: <20180502143323.1c723ccb509c3497050a2e0a@linux-foundation.org>
On 05/02/2018 02:33 PM, Andrew Morton wrote:
> On Tue, 1 May 2018 22:58:06 -0700 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
>
>> For analysis purpose it is useful to have numa node information
>> corresponding mapped address ranges of the process. Currently
>> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
>> allocated per VMA of the process. This is not useful if an user needs to
>> determine which numa node the mapped pages are allocated from for a
>> particular address range. It would have helped if the numa node information
>> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
>> exact numa node from where the pages have been allocated.
>>
>> The format of /proc/<pid>/numa_maps file content is dependent on
>> /proc/<pid>/maps file content as mentioned in the manpage. i.e one line
>> entry for every VMA corresponding to entries in /proc/<pids>/maps file.
>> Therefore changing the output of /proc/<pid>/numa_maps may not be possible.
>>
>> Hence, this patch proposes adding file /proc/<pid>/numa_vamaps which will
>> provide proper break down of VA ranges by numa node id from where the mapped
>> pages are allocated. For Address ranges not having any pages mapped, a '-'
>> is printed instead of numa node id. In addition, this file will include most
>> of the other information currently presented in /proc/<pid>/numa_maps. The
>> additional information included is for convenience. If this is not
>> preferred, the patch could be modified to just provide VA range to numa node
>> information as the rest of the information is already available thru
>> /proc/<pid>/numa_maps file.
>>
>> Since the VA range to numa node information does not include page's PFN,
>> reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
>>
>> Here is the snippet from the new file content showing the format.
>>
>> 00400000-00401000 N0=1 kernelpagesize_kB=4 mapped=1 file=/tmp/hmap2
>> 00600000-00601000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/tmp/hmap2
>> 00601000-00602000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/tmp/hmap2
>> 7f0215600000-7f0215800000 N0=1 kernelpagesize_kB=2048 dirty=1 file=/mnt/f1
>> 7f0215800000-7f0215c00000 - file=/mnt/f1
>> 7f0215c00000-7f0215e00000 N0=1 kernelpagesize_kB=2048 dirty=1 file=/mnt/f1
>> 7f0215e00000-7f0216200000 - file=/mnt/f1
>> ..
>> 7f0217ecb000-7f0217f20000 N0=85 kernelpagesize_kB=4 mapped=85 mapmax=51
>> file=/usr/lib64/libc-2.17.so
>> 7f0217f20000-7f0217f30000 - file=/usr/lib64/libc-2.17.so
>> 7f0217f30000-7f0217f90000 N0=96 kernelpagesize_kB=4 mapped=96 mapmax=51
>> file=/usr/lib64/libc-2.17.so
>> 7f0217f90000-7f0217fb0000 - file=/usr/lib64/libc-2.17.so
>> ..
>>
>> The 'pmap' command can be enhanced to include an option to show numa node
>> information which it can read from this new proc file. This will be a
>> follow on proposal.
> I'd like to hear rather more about the use-cases for this new
> interface. Why do people need it, what is the end-user benefit, etc?
This is mainly for debugging / performance analysis. Oracle Database
team is looking to use this information.
>> There have been couple of previous patch proposals to provide numa node
>> information based on pfn or physical address. They seem to have not made
>> progress. Also it would appear reading numa node information based on PFN
>> or physical address will require privileges(CAP_SYS_ADMIN) similar to
>> reading PFN info from /proc/<pid>/pagemap.
>>
>> See
>> https://marc.info/?t=139630938200001&r=1&w=2
>>
>> https://marc.info/?t=139718724400001&r=1&w=2
> OK, let's hope that these people will be able to provide their review,
> feedback, testing, etc. You missed a couple (Dave, Naoya).
>
>> fs/proc/base.c | 2 +
>> fs/proc/internal.h | 3 +
>> fs/proc/task_mmu.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> Some Documentation/ updates seem appropriate. I suggest you grep the
> directory for "numa_maps" to find suitable locations.
Sure, I can update Documentation/filesystems/proc.txt file which is
where 'numa_maps' is documented.
>
> And a quick build check shows that `size fs/proc/task_mmu.o' gets quite
> a bit larger when CONFIG_SMP=n and CONFIG_NUMA=n. That seems wrong -
> please see if you can eliminate the bloat from systems which don't need
> this feature.
>
>
Ok will take a look.
^ permalink raw reply
* Re: [PATCH] pkeys: Introduce PKEY_ALLOC_SIGNALINHERIT and change signal semantics
From: Ram Pai @ 2018-05-02 23:38 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Florian Weimer, linux-arch, Linux-MM, Linux API, X86 ML,
Dave Hansen, linux-x86_64, linuxppc-dev
In-Reply-To: <CALCETrUfO=vXg5rT-n=y8pLktcq5+ORvgpsOXCHG4GaugB3k2A@mail.gmail.com>
On Wed, May 02, 2018 at 09:18:11PM +0000, Andy Lutomirski wrote:
> On Wed, May 2, 2018 at 2:13 PM Ram Pai <linuxram@us.ibm.com> wrote:
>
>
> > > Ram, would you please comment?
>
> > on POWER the pkey behavior will remain the same at entry or at exit from
> > the signal handler. For eg: if a key is read-disabled on entry into
> > the signal handler, and gets read-enabled in the signal handler, than it
> > will continue to be read-enabled on return from the signal handler.
>
> > In other words, changes to key permissions persist across signal
> > boundaries.
>
> I don't know about POWER's ISA, but this is crappy behavior. If a thread
> temporarily grants itself access to a restrictive memory key and then gets
> a signal, the signal handler should *not* have access to that key.
This is a new requirement that I was not aware off. Its not documented
anywhere AFAICT. Regardless of how the ISA behaves, its still a kernel
behavior that needs to be clearly defined.
--
Ram Pai
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: prakash.sangappa @ 2018-05-02 23:17 UTC (permalink / raw)
To: Dave Hansen, Andrew Morton
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi
In-Reply-To: <2ce01d91-5fba-b1b7-2956-c8cc1853536d@intel.com>
On 05/02/2018 03:28 PM, Dave Hansen wrote:
> On 05/02/2018 02:33 PM, Andrew Morton wrote:
>> On Tue, 1 May 2018 22:58:06 -0700 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
>>> For analysis purpose it is useful to have numa node information
>>> corresponding mapped address ranges of the process. Currently
>>> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
>>> allocated per VMA of the process. This is not useful if an user needs to
>>> determine which numa node the mapped pages are allocated from for a
>>> particular address range. It would have helped if the numa node information
>>> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
>>> exact numa node from where the pages have been allocated.
> I'm finding myself a little lost in figuring out what this does. Today,
> numa_maps might us that a 3-page VMA has 1 page from Node 0 and 2 pages
> from Node 1. We group *entirely* by VMA:
>
> 1000-4000 N0=1 N1=2
Yes
>
> We don't want that. We want to tell exactly where each node's memory is
> despite if they are in the same VMA, like this:
>
> 1000-2000 N1=1
> 2000-3000 N0=1
> 3000-4000 N1=1
>
> So that no line of output ever has more than one node's memory. It
Yes, that is exactly what this patch will provide. It may not have
been clear from the sample output I had included.
Here is another snippet from a process.
..
006dc000-006dd000 N1=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/usr/bin/bash
006dd000-006de000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/usr/bin/bash
006de000-006e0000 N1=2 kernelpagesize_kB=4 anon=2 dirty=2 file=/usr/bin/bash
006e0000-006e6000 N0=6 kernelpagesize_kB=4 anon=6 dirty=6 file=/usr/bin/bash
006e6000-006eb000 N0=5 kernelpagesize_kB=4 anon=5 dirty=5
006eb000-006ec000 N1=1 kernelpagesize_kB=4 anon=1 dirty=1
007f9000-007fa000 N1=1 kernelpagesize_kB=4 anon=1 dirty=1 heap
007fa000-00965000 N0=363 kernelpagesize_kB=4 anon=363 dirty=363 heap
00965000-0096c000 - heap
0096c000-0096d000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 heap
0096d000-00984000 - heap
..
> *appears* in this new file as if each contiguous range of memory from a
> given node has its own VMA. Right?
No. It just breaks down each VMA of the process into address ranges
which have pages on a numa node on each line. i.e Each line will
indicate memory from one numa node only.
>
> This sounds interesting, but I've never found myself wanting this
> information a single time that I can recall. I'd love to hear more.
>
> Is this for debugging? Are apps actually going to *parse* this file?
Yes, mainly for debugging/performance analysis . User analyzing can look
at this file. Oracle Database team will be using this information.
>
> How hard did you try to share code with numa_maps? Are you sure we
> can't just replace numa_maps? VMAs are a kernel-internal thing and we
> never promised to represent them 1:1 in our ABI.
I was inclined to just modify numa_maps. However the man page
documents numa_maps format to correlate with 'maps' file.
Wondering if apps/scripts will break if we change the output
of 'numa_maps'. So decided to add a new file instead.
I could try to share the code with numa_maps.
>
> Are we going to continue creating new files in /proc every time a tiny
> new niche pops up? :)
Wish we could just enhance the existing files.
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Dave Hansen @ 2018-05-02 22:28 UTC (permalink / raw)
To: Andrew Morton, Prakash Sangappa
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi
In-Reply-To: <20180502143323.1c723ccb509c3497050a2e0a@linux-foundation.org>
On 05/02/2018 02:33 PM, Andrew Morton wrote:
> On Tue, 1 May 2018 22:58:06 -0700 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
>> For analysis purpose it is useful to have numa node information
>> corresponding mapped address ranges of the process. Currently
>> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
>> allocated per VMA of the process. This is not useful if an user needs to
>> determine which numa node the mapped pages are allocated from for a
>> particular address range. It would have helped if the numa node information
>> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
>> exact numa node from where the pages have been allocated.
I'm finding myself a little lost in figuring out what this does. Today,
numa_maps might us that a 3-page VMA has 1 page from Node 0 and 2 pages
from Node 1. We group *entirely* by VMA:
1000-4000 N0=1 N1=2
We don't want that. We want to tell exactly where each node's memory is
despite if they are in the same VMA, like this:
1000-2000 N1=1
2000-3000 N0=1
3000-4000 N1=1
So that no line of output ever has more than one node's memory. It
*appears* in this new file as if each contiguous range of memory from a
given node has its own VMA. Right?
This sounds interesting, but I've never found myself wanting this
information a single time that I can recall. I'd love to hear more.
Is this for debugging? Are apps actually going to *parse* this file?
How hard did you try to share code with numa_maps? Are you sure we
can't just replace numa_maps? VMAs are a kernel-internal thing and we
never promised to represent them 1:1 in our ABI.
Are we going to continue creating new files in /proc every time a tiny
new niche pops up? :)
^ permalink raw reply
* Re: [PATCH 4/7] aio: remove the extra get_file/fput pair in io_submit_one
From: Darrick J. Wong @ 2018-05-02 22:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: viro, Avi Kivity, linux-aio, linux-fsdevel, linux-api,
linux-kernel
In-Reply-To: <20180502211448.18276-5-hch@lst.de>
On Wed, May 02, 2018 at 11:14:45PM +0200, Christoph Hellwig wrote:
> If we release the lockdep write protection token before calling into
> ->write_iter and thus never access the file pointer after an -EIOCBQUEUED
> return from ->write_iter or ->read_iter we don't need this extra
> reference.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/aio.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 18507743757a..e5866e2fc331 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1515,16 +1515,19 @@ static ssize_t aio_write(struct kiocb *req, struct iocb *iocb, bool vectored,
> return ret;
> ret = rw_verify_area(WRITE, file, &req->ki_pos, iov_iter_count(&iter));
> if (!ret) {
> - req->ki_flags |= IOCB_WRITE;
> - file_start_write(file);
> - ret = aio_ret(req, call_write_iter(file, req, &iter));
> /*
> - * We release freeze protection in aio_complete(). Fool lockdep
> - * by telling it the lock got released so that it doesn't
> - * complain about held lock when we return to userspace.
> + * Open-code file_start_write here to grab freeze protection,
> + * which will be released by another thread in aio_complete().
> + * Fool lockdep by telling it the lock got released so that it
> + * doesn't complain about the held lock when we return to
> + * userspace.
> */
> - if (S_ISREG(file_inode(file)->i_mode))
> + if (S_ISREG(file_inode(file)->i_mode)) {
> + __sb_start_write(file_inode(file)->i_sb, SB_FREEZE_WRITE, true);
> __sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> + }
> + req->ki_flags |= IOCB_WRITE;
> + ret = aio_ret(req, call_write_iter(file, req, &iter));
> }
> kfree(iovec);
> return ret;
> @@ -1599,7 +1602,6 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
> req->ki_user_iocb = user_iocb;
> req->ki_user_data = iocb->aio_data;
>
> - get_file(file);
> switch (iocb->aio_lio_opcode) {
> case IOCB_CMD_PREAD:
> ret = aio_read(&req->common, iocb, false, compat);
> @@ -1618,8 +1620,13 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
> ret = -EINVAL;
> break;
> }
> - fput(file);
>
> + /*
> + * If ret is -EIOCBQUEUED, ownership of the file reference acquired
> + * above passed to the file system, which at this point might have
> + * dropped the reference, so we must be careful to not reference it
> + * once we have called into the file system.
> + */
> if (ret && ret != -EIOCBQUEUED)
> goto out_put_req;
> return 0;
> --
> 2.17.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-api" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [RFC PATCH] Add /proc/<pid>/numa_vamaps for numa node information
From: Andrew Morton @ 2018-05-02 21:33 UTC (permalink / raw)
To: Prakash Sangappa
Cc: linux-kernel, linux-mm, linux-api, mhocko, kirill.shutemov,
n-horiguchi, drepper, rientjes, Naoya Horiguchi, Dave Hansen
In-Reply-To: <1525240686-13335-1-git-send-email-prakash.sangappa@oracle.com>
On Tue, 1 May 2018 22:58:06 -0700 Prakash Sangappa <prakash.sangappa@oracle.com> wrote:
> For analysis purpose it is useful to have numa node information
> corresponding mapped address ranges of the process. Currently
> /proc/<pid>/numa_maps provides list of numa nodes from where pages are
> allocated per VMA of the process. This is not useful if an user needs to
> determine which numa node the mapped pages are allocated from for a
> particular address range. It would have helped if the numa node information
> presented in /proc/<pid>/numa_maps was broken down by VA ranges showing the
> exact numa node from where the pages have been allocated.
>
> The format of /proc/<pid>/numa_maps file content is dependent on
> /proc/<pid>/maps file content as mentioned in the manpage. i.e one line
> entry for every VMA corresponding to entries in /proc/<pids>/maps file.
> Therefore changing the output of /proc/<pid>/numa_maps may not be possible.
>
> Hence, this patch proposes adding file /proc/<pid>/numa_vamaps which will
> provide proper break down of VA ranges by numa node id from where the mapped
> pages are allocated. For Address ranges not having any pages mapped, a '-'
> is printed instead of numa node id. In addition, this file will include most
> of the other information currently presented in /proc/<pid>/numa_maps. The
> additional information included is for convenience. If this is not
> preferred, the patch could be modified to just provide VA range to numa node
> information as the rest of the information is already available thru
> /proc/<pid>/numa_maps file.
>
> Since the VA range to numa node information does not include page's PFN,
> reading this file will not be restricted(i.e requiring CAP_SYS_ADMIN).
>
> Here is the snippet from the new file content showing the format.
>
> 00400000-00401000 N0=1 kernelpagesize_kB=4 mapped=1 file=/tmp/hmap2
> 00600000-00601000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/tmp/hmap2
> 00601000-00602000 N0=1 kernelpagesize_kB=4 anon=1 dirty=1 file=/tmp/hmap2
> 7f0215600000-7f0215800000 N0=1 kernelpagesize_kB=2048 dirty=1 file=/mnt/f1
> 7f0215800000-7f0215c00000 - file=/mnt/f1
> 7f0215c00000-7f0215e00000 N0=1 kernelpagesize_kB=2048 dirty=1 file=/mnt/f1
> 7f0215e00000-7f0216200000 - file=/mnt/f1
> ..
> 7f0217ecb000-7f0217f20000 N0=85 kernelpagesize_kB=4 mapped=85 mapmax=51
> file=/usr/lib64/libc-2.17.so
> 7f0217f20000-7f0217f30000 - file=/usr/lib64/libc-2.17.so
> 7f0217f30000-7f0217f90000 N0=96 kernelpagesize_kB=4 mapped=96 mapmax=51
> file=/usr/lib64/libc-2.17.so
> 7f0217f90000-7f0217fb0000 - file=/usr/lib64/libc-2.17.so
> ..
>
> The 'pmap' command can be enhanced to include an option to show numa node
> information which it can read from this new proc file. This will be a
> follow on proposal.
I'd like to hear rather more about the use-cases for this new
interface. Why do people need it, what is the end-user benefit, etc?
> There have been couple of previous patch proposals to provide numa node
> information based on pfn or physical address. They seem to have not made
> progress. Also it would appear reading numa node information based on PFN
> or physical address will require privileges(CAP_SYS_ADMIN) similar to
> reading PFN info from /proc/<pid>/pagemap.
>
> See
> https://marc.info/?t=139630938200001&r=1&w=2
>
> https://marc.info/?t=139718724400001&r=1&w=2
OK, let's hope that these people will be able to provide their review,
feedback, testing, etc. You missed a couple (Dave, Naoya).
> fs/proc/base.c | 2 +
> fs/proc/internal.h | 3 +
> fs/proc/task_mmu.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
Some Documentation/ updates seem appropriate. I suggest you grep the
directory for "numa_maps" to find suitable locations.
And a quick build check shows that `size fs/proc/task_mmu.o' gets quite
a bit larger when CONFIG_SMP=n and CONFIG_NUMA=n. That seems wrong -
please see if you can eliminate the bloat from systems which don't need
this feature.
^ permalink raw reply
* [PATCH 7/7] aio: implement io_pgetevents
From: Christoph Hellwig @ 2018-05-02 21:14 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180502211448.18276-1-hch@lst.de>
This is the io_getevents equivalent of ppoll/pselect and allows to
properly mix signals and aio completions (especially with IOCB_CMD_POLL)
and atomically executes the following sequence:
sigset_t origmask;
pthread_sigmask(SIG_SETMASK, &sigmask, &origmask);
ret = io_getevents(ctx, min_nr, nr, events, timeout);
pthread_sigmask(SIG_SETMASK, &origmask, NULL);
Note that unlike many other signal related calls we do not pass a sigmask
size, as that would get us to 7 arguments, which aren't easily supported
by the syscall infrastructure. It seems a lot less painful to just add a
new syscall variant in the unlikely case we're going to increase the
sigset size.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
fs/aio.c | 114 ++++++++++++++++++++++---
include/linux/compat.h | 7 ++
include/linux/syscalls.h | 6 ++
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/aio_abi.h | 6 ++
kernel/sys_ni.c | 2 +
8 files changed, 130 insertions(+), 11 deletions(-)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index d6b27dab1b30..14a2f996e543 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -396,3 +396,4 @@
382 i386 pkey_free sys_pkey_free __ia32_sys_pkey_free
383 i386 statx sys_statx __ia32_sys_statx
384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
+385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 4dfe42666d0c..cd36232ab62f 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -341,6 +341,7 @@
330 common pkey_alloc __x64_sys_pkey_alloc
331 common pkey_free __x64_sys_pkey_free
332 common statx __x64_sys_statx
+333 common io_pgetevents __x64_sys_io_pgetevents
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/aio.c b/fs/aio.c
index 61d2e6942951..f3eae5d5771b 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1303,10 +1303,6 @@ static long read_events(struct kioctx *ctx, long min_nr, long nr,
wait_event_interruptible_hrtimeout(ctx->wait,
aio_read_events(ctx, min_nr, nr, event, &ret),
until);
-
- if (!ret && signal_pending(current))
- ret = -EINTR;
-
return ret;
}
@@ -1921,13 +1917,60 @@ SYSCALL_DEFINE5(io_getevents, aio_context_t, ctx_id,
struct timespec __user *, timeout)
{
struct timespec64 ts;
+ int ret;
+
+ if (timeout && unlikely(get_timespec64(&ts, timeout)))
+ return -EFAULT;
+
+ ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
+ if (!ret && signal_pending(current))
+ ret = -EINTR;
+ return ret;
+}
- if (timeout) {
- if (unlikely(get_timespec64(&ts, timeout)))
+SYSCALL_DEFINE6(io_pgetevents,
+ aio_context_t, ctx_id,
+ long, min_nr,
+ long, nr,
+ struct io_event __user *, events,
+ struct timespec __user *, timeout,
+ const struct __aio_sigset __user *, usig)
+{
+ struct __aio_sigset ksig = { NULL, };
+ sigset_t ksigmask, sigsaved;
+ struct timespec64 ts;
+ int ret;
+
+ if (timeout && unlikely(get_timespec64(&ts, timeout)))
+ return -EFAULT;
+
+ if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
+ return -EFAULT;
+
+ if (ksig.sigmask) {
+ if (ksig.sigsetsize != sizeof(sigset_t))
+ return -EINVAL;
+ if (copy_from_user(&ksigmask, ksig.sigmask, sizeof(ksigmask)))
return -EFAULT;
+ sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
+ sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
+ }
+
+ ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
+ if (signal_pending(current)) {
+ if (ksig.sigmask) {
+ current->saved_sigmask = sigsaved;
+ set_restore_sigmask();
+ }
+
+ if (!ret)
+ ret = -ERESTARTNOHAND;
+ } else {
+ if (ksig.sigmask)
+ sigprocmask(SIG_SETMASK, &sigsaved, NULL);
}
- return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &ts : NULL);
+ return ret;
}
#ifdef CONFIG_COMPAT
@@ -1938,13 +1981,64 @@ COMPAT_SYSCALL_DEFINE5(io_getevents, compat_aio_context_t, ctx_id,
struct compat_timespec __user *, timeout)
{
struct timespec64 t;
+ int ret;
+
+ if (timeout && compat_get_timespec64(&t, timeout))
+ return -EFAULT;
+
+ ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
+ if (!ret && signal_pending(current))
+ ret = -EINTR;
+ return ret;
+}
+
- if (timeout) {
- if (compat_get_timespec64(&t, timeout))
+struct __compat_aio_sigset {
+ compat_sigset_t __user *sigmask;
+ compat_size_t sigsetsize;
+};
+
+COMPAT_SYSCALL_DEFINE6(io_pgetevents,
+ compat_aio_context_t, ctx_id,
+ compat_long_t, min_nr,
+ compat_long_t, nr,
+ struct io_event __user *, events,
+ struct compat_timespec __user *, timeout,
+ const struct __compat_aio_sigset __user *, usig)
+{
+ struct __compat_aio_sigset ksig = { NULL, };
+ sigset_t ksigmask, sigsaved;
+ struct timespec64 t;
+ int ret;
+
+ if (timeout && compat_get_timespec64(&t, timeout))
+ return -EFAULT;
+
+ if (usig && copy_from_user(&ksig, usig, sizeof(ksig)))
+ return -EFAULT;
+
+ if (ksig.sigmask) {
+ if (ksig.sigsetsize != sizeof(compat_sigset_t))
+ return -EINVAL;
+ if (get_compat_sigset(&ksigmask, ksig.sigmask))
return -EFAULT;
+ sigdelsetmask(&ksigmask, sigmask(SIGKILL) | sigmask(SIGSTOP));
+ sigprocmask(SIG_SETMASK, &ksigmask, &sigsaved);
+ }
+ ret = do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
+ if (signal_pending(current)) {
+ if (ksig.sigmask) {
+ current->saved_sigmask = sigsaved;
+ set_restore_sigmask();
+ }
+ if (!ret)
+ ret = -ERESTARTNOHAND;
+ } else {
+ if (ksig.sigmask)
+ sigprocmask(SIG_SETMASK, &sigsaved, NULL);
}
- return do_io_getevents(ctx_id, min_nr, nr, events, timeout ? &t : NULL);
+ return ret;
}
#endif
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 081281ad5772..ad192057b887 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -330,6 +330,7 @@ extern int put_compat_rusage(const struct rusage *,
struct compat_rusage __user *);
struct compat_siginfo;
+struct __compat_aio_sigset;
struct compat_dirent {
u32 d_ino;
@@ -553,6 +554,12 @@ asmlinkage long compat_sys_io_getevents(compat_aio_context_t ctx_id,
compat_long_t nr,
struct io_event __user *events,
struct compat_timespec __user *timeout);
+asmlinkage long compat_sys_io_pgetevents(compat_aio_context_t ctx_id,
+ compat_long_t min_nr,
+ compat_long_t nr,
+ struct io_event __user *events,
+ struct compat_timespec __user *timeout,
+ const struct __compat_aio_sigset __user *usig);
/* fs/cookies.c */
asmlinkage long compat_sys_lookup_dcookie(u32, u32, char __user *, compat_size_t);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 70fcda1a9049..811172fcb916 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -290,6 +290,12 @@ asmlinkage long sys_io_getevents(aio_context_t ctx_id,
long nr,
struct io_event __user *events,
struct timespec __user *timeout);
+asmlinkage long sys_io_pgetevents(aio_context_t ctx_id,
+ long min_nr,
+ long nr,
+ struct io_event __user *events,
+ struct timespec __user *timeout,
+ const struct __aio_sigset *sig);
/* fs/xattr.c */
asmlinkage long sys_setxattr(const char __user *path, const char __user *name,
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 8bcb186c6f67..42990676a55e 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -732,9 +732,11 @@ __SYSCALL(__NR_pkey_alloc, sys_pkey_alloc)
__SYSCALL(__NR_pkey_free, sys_pkey_free)
#define __NR_statx 291
__SYSCALL(__NR_statx, sys_statx)
+#define __NR_io_pgetevents 292
+__SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
#undef __NR_syscalls
-#define __NR_syscalls 292
+#define __NR_syscalls 293
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/aio_abi.h b/include/uapi/linux/aio_abi.h
index a04adbc70ddf..2c0a3415beee 100644
--- a/include/uapi/linux/aio_abi.h
+++ b/include/uapi/linux/aio_abi.h
@@ -29,6 +29,7 @@
#include <linux/types.h>
#include <linux/fs.h>
+#include <linux/signal.h>
#include <asm/byteorder.h>
typedef __kernel_ulong_t aio_context_t;
@@ -108,5 +109,10 @@ struct iocb {
#undef IFBIG
#undef IFLITTLE
+struct __aio_sigset {
+ sigset_t __user *sigmask;
+ size_t sigsetsize;
+};
+
#endif /* __LINUX__AIO_ABI_H */
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 9791364925dc..183169c2a75b 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -43,7 +43,9 @@ COND_SYSCALL(io_submit);
COND_SYSCALL_COMPAT(io_submit);
COND_SYSCALL(io_cancel);
COND_SYSCALL(io_getevents);
+COND_SYSCALL(io_pgetevents);
COND_SYSCALL_COMPAT(io_getevents);
+COND_SYSCALL_COMPAT(io_pgetevents);
/* fs/xattr.c */
--
2.17.0
^ permalink raw reply related
* [PATCH 6/7] aio: implement IOCB_CMD_FSYNC and IOCB_CMD_FDSYNC
From: Christoph Hellwig @ 2018-05-02 21:14 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180502211448.18276-1-hch@lst.de>
Simple workqueue offload for now, but prepared for adding a real aio_fsync
method if the need arises. Based on an earlier patch from Dave Chinner.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/aio.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/fs/aio.c b/fs/aio.c
index d07c27b3fe88..61d2e6942951 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -156,6 +156,12 @@ struct kioctx {
unsigned id;
};
+struct fsync_iocb {
+ struct work_struct work;
+ struct file *file;
+ bool datasync;
+};
+
/*
* We use ki_cancel == KIOCB_CANCELLED to indicate that a kiocb has been either
* cancelled or completed (this makes a certain amount of sense because
@@ -172,6 +178,7 @@ struct kioctx {
struct aio_kiocb {
union {
struct kiocb rw;
+ struct fsync_iocb fsync;
};
struct kioctx *ki_ctx;
@@ -1573,6 +1580,36 @@ static ssize_t aio_write(struct kiocb *req, struct iocb *iocb, bool vectored,
return ret;
}
+static void aio_fsync_work(struct work_struct *work)
+{
+ struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
+ int ret;
+
+ ret = vfs_fsync(req->file, req->datasync);
+ fput(req->file);
+ aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
+}
+
+static int aio_fsync(struct fsync_iocb *req, struct iocb *iocb, bool datasync)
+{
+ if (unlikely(iocb->aio_buf || iocb->aio_offset || iocb->aio_nbytes ||
+ iocb->aio_rw_flags))
+ return -EINVAL;
+
+ req->file = fget(iocb->aio_fildes);
+ if (unlikely(!req->file))
+ return -EBADF;
+ if (unlikely(!req->file->f_op->fsync)) {
+ fput(req->file);
+ return -EINVAL;
+ }
+
+ req->datasync = datasync;
+ INIT_WORK(&req->work, aio_fsync_work);
+ schedule_work(&req->work);
+ return -EIOCBQUEUED;
+}
+
static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
struct iocb *iocb, bool compat)
{
@@ -1636,6 +1673,12 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
case IOCB_CMD_PWRITEV:
ret = aio_write(&req->rw, iocb, true, compat);
break;
+ case IOCB_CMD_FSYNC:
+ ret = aio_fsync(&req->fsync, iocb, false);
+ break;
+ case IOCB_CMD_FDSYNC:
+ ret = aio_fsync(&req->fsync, iocb, true);
+ break;
default:
pr_debug("invalid aio operation %d\n", iocb->aio_lio_opcode);
ret = -EINVAL;
--
2.17.0
^ permalink raw reply related
* [PATCH 5/7] aio: refactor read/write iocb setup
From: Christoph Hellwig @ 2018-05-02 21:14 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180502211448.18276-1-hch@lst.de>
Don't reference the kiocb structure from the common aio code, and move
any use of it into helper specific to the read/write path. This is in
preparation for aio_poll support that wants to use the space for different
fields.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/aio.c | 161 +++++++++++++++++++++++++++++++------------------------
1 file changed, 92 insertions(+), 69 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index e5866e2fc331..d07c27b3fe88 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -170,7 +170,9 @@ struct kioctx {
#define KIOCB_CANCELLED ((void *) (~0ULL))
struct aio_kiocb {
- struct kiocb common;
+ union {
+ struct kiocb rw;
+ };
struct kioctx *ki_ctx;
kiocb_cancel_fn *ki_cancel;
@@ -549,7 +551,7 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events)
void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
{
- struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, common);
+ struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw);
struct kioctx *ctx = req->ki_ctx;
unsigned long flags;
@@ -581,7 +583,7 @@ static int kiocb_cancel(struct aio_kiocb *kiocb)
cancel = cmpxchg(&kiocb->ki_cancel, old, KIOCB_CANCELLED);
} while (cancel != old);
- return cancel(&kiocb->common);
+ return cancel(&kiocb->rw);
}
/*
@@ -1046,15 +1048,6 @@ static inline struct aio_kiocb *aio_get_req(struct kioctx *ctx)
return NULL;
}
-static void kiocb_free(struct aio_kiocb *req)
-{
- if (req->common.ki_filp)
- fput(req->common.ki_filp);
- if (req->ki_eventfd != NULL)
- eventfd_ctx_put(req->ki_eventfd);
- kmem_cache_free(kiocb_cachep, req);
-}
-
static struct kioctx *lookup_ioctx(unsigned long ctx_id)
{
struct aio_ring __user *ring = (void __user *)ctx_id;
@@ -1085,27 +1078,14 @@ static struct kioctx *lookup_ioctx(unsigned long ctx_id)
/* aio_complete
* Called when the io request on the given iocb is complete.
*/
-static void aio_complete(struct kiocb *kiocb, long res, long res2)
+static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
{
- struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, common);
struct kioctx *ctx = iocb->ki_ctx;
struct aio_ring *ring;
struct io_event *ev_page, *event;
unsigned tail, pos, head;
unsigned long flags;
- if (kiocb->ki_flags & IOCB_WRITE) {
- struct file *file = kiocb->ki_filp;
-
- /*
- * Tell lockdep we inherited freeze protection from submission
- * thread.
- */
- if (S_ISREG(file_inode(file)->i_mode))
- __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
- file_end_write(file);
- }
-
if (!list_empty_careful(&iocb->ki_list)) {
unsigned long flags;
@@ -1167,11 +1147,12 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
* eventfd. The eventfd_signal() function is safe to be called
* from IRQ context.
*/
- if (iocb->ki_eventfd != NULL)
+ if (iocb->ki_eventfd) {
eventfd_signal(iocb->ki_eventfd, 1);
+ eventfd_ctx_put(iocb->ki_eventfd);
+ }
- /* everything turned out well, dispose of the aiocb. */
- kiocb_free(iocb);
+ kmem_cache_free(kiocb_cachep, iocb);
/*
* We have to order our ring_info tail store above and test
@@ -1434,6 +1415,45 @@ SYSCALL_DEFINE1(io_destroy, aio_context_t, ctx)
return -EINVAL;
}
+static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
+{
+ struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, rw);
+
+ if (kiocb->ki_flags & IOCB_WRITE) {
+ struct inode *inode = file_inode(kiocb->ki_filp);
+
+ /*
+ * Tell lockdep we inherited freeze protection from submission
+ * thread.
+ */
+ if (S_ISREG(inode->i_mode))
+ __sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
+ file_end_write(kiocb->ki_filp);
+ }
+
+ fput(kiocb->ki_filp);
+ aio_complete(iocb, res, res2);
+}
+
+static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
+{
+ int ret;
+
+ req->ki_filp = fget(iocb->aio_fildes);
+ if (unlikely(!req->ki_filp))
+ return -EBADF;
+ req->ki_complete = aio_complete_rw;
+ req->ki_pos = iocb->aio_offset;
+ req->ki_flags = iocb_flags(req->ki_filp);
+ if (iocb->aio_flags & IOCB_FLAG_RESFD)
+ req->ki_flags |= IOCB_EVENTFD;
+ req->ki_hint = file_write_hint(req->ki_filp);
+ ret = kiocb_set_rw_flags(req, iocb->aio_rw_flags);
+ if (unlikely(ret))
+ fput(req->ki_filp);
+ return ret;
+}
+
static int aio_setup_rw(int rw, struct iocb *iocb, struct iovec **iovec,
bool vectored, bool compat, struct iov_iter *iter)
{
@@ -1453,7 +1473,7 @@ static int aio_setup_rw(int rw, struct iocb *iocb, struct iovec **iovec,
return import_iovec(rw, buf, len, UIO_FASTIOV, iovec, iter);
}
-static inline ssize_t aio_ret(struct kiocb *req, ssize_t ret)
+static inline ssize_t aio_rw_ret(struct kiocb *req, ssize_t ret)
{
switch (ret) {
case -EIOCBQUEUED:
@@ -1469,7 +1489,7 @@ static inline ssize_t aio_ret(struct kiocb *req, ssize_t ret)
ret = -EINTR;
/*FALLTHRU*/
default:
- aio_complete(req, ret, 0);
+ aio_complete_rw(req, ret, 0);
return 0;
}
}
@@ -1477,59 +1497,79 @@ static inline ssize_t aio_ret(struct kiocb *req, ssize_t ret)
static ssize_t aio_read(struct kiocb *req, struct iocb *iocb, bool vectored,
bool compat)
{
- struct file *file = req->ki_filp;
struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
struct iov_iter iter;
+ struct file *file;
ssize_t ret;
+ ret = aio_prep_rw(req, iocb);
+ if (ret)
+ return ret;
+ file = req->ki_filp;
+
+ ret = -EBADF;
if (unlikely(!(file->f_mode & FMODE_READ)))
- return -EBADF;
+ goto out_fput;
+ ret = -EINVAL;
if (unlikely(!file->f_op->read_iter))
- return -EINVAL;
+ goto out_fput;
ret = aio_setup_rw(READ, iocb, &iovec, vectored, compat, &iter);
if (ret)
- return ret;
+ goto out_fput;
ret = rw_verify_area(READ, file, &req->ki_pos, iov_iter_count(&iter));
if (!ret)
- ret = aio_ret(req, call_read_iter(file, req, &iter));
+ ret = aio_rw_ret(req, call_read_iter(file, req, &iter));
kfree(iovec);
+out_fput:
+ if (unlikely(ret && ret != -EIOCBQUEUED))
+ fput(file);
return ret;
}
static ssize_t aio_write(struct kiocb *req, struct iocb *iocb, bool vectored,
bool compat)
{
- struct file *file = req->ki_filp;
struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
struct iov_iter iter;
+ struct file *file;
ssize_t ret;
+ ret = aio_prep_rw(req, iocb);
+ if (ret)
+ return ret;
+ file = req->ki_filp;
+
+ ret = -EBADF;
if (unlikely(!(file->f_mode & FMODE_WRITE)))
- return -EBADF;
+ goto out_fput;
+ ret = -EINVAL;
if (unlikely(!file->f_op->write_iter))
- return -EINVAL;
+ goto out_fput;
ret = aio_setup_rw(WRITE, iocb, &iovec, vectored, compat, &iter);
if (ret)
- return ret;
+ goto out_fput;
ret = rw_verify_area(WRITE, file, &req->ki_pos, iov_iter_count(&iter));
if (!ret) {
/*
* Open-code file_start_write here to grab freeze protection,
- * which will be released by another thread in aio_complete().
- * Fool lockdep by telling it the lock got released so that it
- * doesn't complain about the held lock when we return to
- * userspace.
+ * which will be released by another thread in
+ * aio_complete_rw(). Fool lockdep by telling it the lock got
+ * released so that it doesn't complain about the held lock when
+ * we return to userspace.
*/
if (S_ISREG(file_inode(file)->i_mode)) {
__sb_start_write(file_inode(file)->i_sb, SB_FREEZE_WRITE, true);
__sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
}
req->ki_flags |= IOCB_WRITE;
- ret = aio_ret(req, call_write_iter(file, req, &iter));
+ ret = aio_rw_ret(req, call_write_iter(file, req, &iter));
}
kfree(iovec);
+out_fput:
+ if (unlikely(ret && ret != -EIOCBQUEUED))
+ fput(file);
return ret;
}
@@ -1537,7 +1577,6 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
struct iocb *iocb, bool compat)
{
struct aio_kiocb *req;
- struct file *file;
ssize_t ret;
/* enforce forwards compatibility on users */
@@ -1560,16 +1599,6 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
if (unlikely(!req))
return -EAGAIN;
- req->common.ki_filp = file = fget(iocb->aio_fildes);
- if (unlikely(!req->common.ki_filp)) {
- ret = -EBADF;
- goto out_put_req;
- }
- req->common.ki_pos = iocb->aio_offset;
- req->common.ki_complete = aio_complete;
- req->common.ki_flags = iocb_flags(req->common.ki_filp);
- req->common.ki_hint = file_write_hint(file);
-
if (iocb->aio_flags & IOCB_FLAG_RESFD) {
/*
* If the IOCB_FLAG_RESFD flag of aio_flags is set, get an
@@ -1583,14 +1612,6 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
req->ki_eventfd = NULL;
goto out_put_req;
}
-
- req->common.ki_flags |= IOCB_EVENTFD;
- }
-
- ret = kiocb_set_rw_flags(&req->common, iocb->aio_rw_flags);
- if (unlikely(ret)) {
- pr_debug("EINVAL: aio_rw_flags\n");
- goto out_put_req;
}
ret = put_user(KIOCB_KEY, &user_iocb->aio_key);
@@ -1604,16 +1625,16 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
switch (iocb->aio_lio_opcode) {
case IOCB_CMD_PREAD:
- ret = aio_read(&req->common, iocb, false, compat);
+ ret = aio_read(&req->rw, iocb, false, compat);
break;
case IOCB_CMD_PWRITE:
- ret = aio_write(&req->common, iocb, false, compat);
+ ret = aio_write(&req->rw, iocb, false, compat);
break;
case IOCB_CMD_PREADV:
- ret = aio_read(&req->common, iocb, true, compat);
+ ret = aio_read(&req->rw, iocb, true, compat);
break;
case IOCB_CMD_PWRITEV:
- ret = aio_write(&req->common, iocb, true, compat);
+ ret = aio_write(&req->rw, iocb, true, compat);
break;
default:
pr_debug("invalid aio operation %d\n", iocb->aio_lio_opcode);
@@ -1633,7 +1654,9 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
out_put_req:
put_reqs_available(ctx, 1);
percpu_ref_put(&ctx->reqs);
- kiocb_free(req);
+ if (req->ki_eventfd)
+ eventfd_ctx_put(req->ki_eventfd);
+ kmem_cache_free(kiocb_cachep, req);
return ret;
}
--
2.17.0
^ permalink raw reply related
* [PATCH 4/7] aio: remove the extra get_file/fput pair in io_submit_one
From: Christoph Hellwig @ 2018-05-02 21:14 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180502211448.18276-1-hch@lst.de>
If we release the lockdep write protection token before calling into
->write_iter and thus never access the file pointer after an -EIOCBQUEUED
return from ->write_iter or ->read_iter we don't need this extra
reference.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/aio.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index 18507743757a..e5866e2fc331 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1515,16 +1515,19 @@ static ssize_t aio_write(struct kiocb *req, struct iocb *iocb, bool vectored,
return ret;
ret = rw_verify_area(WRITE, file, &req->ki_pos, iov_iter_count(&iter));
if (!ret) {
- req->ki_flags |= IOCB_WRITE;
- file_start_write(file);
- ret = aio_ret(req, call_write_iter(file, req, &iter));
/*
- * We release freeze protection in aio_complete(). Fool lockdep
- * by telling it the lock got released so that it doesn't
- * complain about held lock when we return to userspace.
+ * Open-code file_start_write here to grab freeze protection,
+ * which will be released by another thread in aio_complete().
+ * Fool lockdep by telling it the lock got released so that it
+ * doesn't complain about the held lock when we return to
+ * userspace.
*/
- if (S_ISREG(file_inode(file)->i_mode))
+ if (S_ISREG(file_inode(file)->i_mode)) {
+ __sb_start_write(file_inode(file)->i_sb, SB_FREEZE_WRITE, true);
__sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
+ }
+ req->ki_flags |= IOCB_WRITE;
+ ret = aio_ret(req, call_write_iter(file, req, &iter));
}
kfree(iovec);
return ret;
@@ -1599,7 +1602,6 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
req->ki_user_iocb = user_iocb;
req->ki_user_data = iocb->aio_data;
- get_file(file);
switch (iocb->aio_lio_opcode) {
case IOCB_CMD_PREAD:
ret = aio_read(&req->common, iocb, false, compat);
@@ -1618,8 +1620,13 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
ret = -EINVAL;
break;
}
- fput(file);
+ /*
+ * If ret is -EIOCBQUEUED, ownership of the file reference acquired
+ * above passed to the file system, which at this point might have
+ * dropped the reference, so we must be careful to not reference it
+ * once we have called into the file system.
+ */
if (ret && ret != -EIOCBQUEUED)
goto out_put_req;
return 0;
--
2.17.0
^ permalink raw reply related
* [PATCH 3/7] aio: sanitize ki_list handling
From: Christoph Hellwig @ 2018-05-02 21:14 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180502211448.18276-1-hch@lst.de>
Instead of handcoded non-null checks always initialize ki_list to an
empty list and use list_empty / list_empty_careful on it. While we're
at it also error out on a double call to kiocb_set_cancel_fn instead
of ignoring it.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/aio.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index 7c1855afd723..18507743757a 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -553,13 +553,12 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
struct kioctx *ctx = req->ki_ctx;
unsigned long flags;
- spin_lock_irqsave(&ctx->ctx_lock, flags);
-
- if (!req->ki_list.next)
- list_add(&req->ki_list, &ctx->active_reqs);
+ if (WARN_ON_ONCE(!list_empty(&req->ki_list)))
+ return;
+ spin_lock_irqsave(&ctx->ctx_lock, flags);
+ list_add_tail(&req->ki_list, &ctx->active_reqs);
req->ki_cancel = cancel;
-
spin_unlock_irqrestore(&ctx->ctx_lock, flags);
}
EXPORT_SYMBOL(kiocb_set_cancel_fn);
@@ -1039,7 +1038,7 @@ static inline struct aio_kiocb *aio_get_req(struct kioctx *ctx)
goto out_put;
percpu_ref_get(&ctx->reqs);
-
+ INIT_LIST_HEAD(&req->ki_list);
req->ki_ctx = ctx;
return req;
out_put:
@@ -1107,7 +1106,7 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
file_end_write(file);
}
- if (iocb->ki_list.next) {
+ if (!list_empty_careful(&iocb->ki_list)) {
unsigned long flags;
spin_lock_irqsave(&ctx->ctx_lock, flags);
--
2.17.0
^ permalink raw reply related
* [PATCH 2/7] aio: remove an outdated BUG_ON and comment in aio_complete
From: Christoph Hellwig @ 2018-05-02 21:14 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <20180502211448.18276-1-hch@lst.de>
These days we don't treat sync iocbs special in the aio completion code as
they never use it. Remove the old comment and BUG_ON given that the
current definition of is_sync_kiocb makes it impossible to hit.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
fs/aio.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index add46b06be86..7c1855afd723 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1107,15 +1107,6 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
file_end_write(file);
}
- /*
- * Special case handling for sync iocbs:
- * - events go directly into the iocb for fast handling
- * - the sync task with the iocb in its stack holds the single iocb
- * ref, no other paths have a way to get another ref
- * - the sync task helpfully left a reference to itself in the iocb
- */
- BUG_ON(is_sync_kiocb(kiocb));
-
if (iocb->ki_list.next) {
unsigned long flags;
--
2.17.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox