* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Jonathan Corbet @ 2018-03-29 13:03 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, luto, jlayton, carlos, viro,
dhowells, simo, eparis, serge, ebiederm, madzcar
In-Reply-To: <20180329090132.r3qfomigkw3hbwbw@madcap2.tricolour.ca>
On Thu, 29 Mar 2018 05:01:32 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:
> > A little detail, but still...
>
> I am understanding that you would prefer more context (as opposed to
> operational detail) in the description, laying out the use case for this
> patch(set)?
No, sorry, "a little detail" was referring to my comment. The use case,
I believe, has been well described.
Thanks,
jon
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-29 13:54 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <wil>
In-Reply-To: <alpine.DEB.2.21.1803282305030.1496@nanos.tec.linutronix.de>
----- On Mar 28, 2018, at 5:25 PM, Thomas Gleixner tglx@linutronix.de wrote:
> On Wed, 28 Mar 2018, Mathieu Desnoyers wrote:
>> ----- On Mar 28, 2018, at 1:49 PM, Peter Zijlstra peterz@infradead.org wrote:
>> > I don't think disallowing system calls is arbitrary. And I think that is
>> > something we really want to enforce, because it's batshit insane to
>> > allow.
>> >
>> > And if we allow now, people _will_ use it and we can't ever take it
>> > away again.
>>
>> Here are some examples of how I would like to use system calls within
>> rseq critical sections, for testing purposes:
>>
>> - Issue poll(NULL, 0, ms_timeout) from a rseq critical section, to introduce
>> a delay in the critical section and test the effect,
>
> It's simple enough to use a delay loop for that. It's testing after all.
>
>> - Issue sched_yield() from a rseq critical section, to introduce preemption at
>> that point,
>
> Make it loop on a varible and use secondary threads to force preemption.
>
>> - Issue kill() on self, thus testing interruption by signals over rseq c.s.,
>
> Second thread can do that
>
>> - Invoke sched_setaffinity to tweak the cpu affinity mask to force thread
>> migration within a rseq c.s.
>
> Second thread can do that
>
>> I currently have only implemented the poll(), sched_yield() and kill()
>> test-cases outside of the rseq critical sections, instead relying on
>> assembly loops to introduce delays in rseq c.s.. However, if we disallow
>> system calls in rseq critical sections, I'll never be able to use those
>> systems calls to extend the test matrix.
>
> All of these tests can be implemented without system calls and there is no
> justification to allow system calls just because it makes writing test
> cases simpler. Nice try.
You bring good points. As a logical consequence, I indeed don't need to issue
system calls from rseq c.s. for testing.
>
>> I see other use-cases where having a system call in a rseq critical section
>> could make sense: if vDSO data shared between kernel and user-space rely
>> on rseq for synchronization, but a fallback sometimes needs to issue a system
>> call for part of the operation.
>
> What in the VDSO relies on rseqs? Nothing AFAICT. If the VDSO ever goes to
> use that then it's going to be a kernel/vdso specific variant and we'll
> figure out how that needs to be handled if at all.
Sure, and we can craft the vDSO so the system call does not need to be
issued within a rseq c.s.. So this one is a non-issue I think.
>
> But we are not misdesigning now to accomodate artificial scenarios dreamed
> up for argumentation sake,
If we decide to impose limitations on the rseq c.s. abilities, we need to
think this through very carefully.
Let's say we disallow system calls from rseq critical sections. A few points
arise:
- We still need to allow traps (page faults, breakpoints, ...) within rseq c.s.,
- We still need to allow interrupts within rseq c.s.,
- We need to decide whether we just document that syscalls within rseq c.s.
are not supported, or we enforce a behavior if this happens (e.g. SIGSEGV).
If we enforce a SIGSEGV, we'd have to figure out whether it's worth it to
add extra branches to the system call fast path to validate this.
- If we document that syscalls are not supported within rseq c.s., we should
specify whether doing so terminates the process, or if it merely does not
guarantee proper abort behavior of the critical section.
- We need to carefully consider the case of system calls issued within signal
handlers nested on top of rseq. When RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL is
_not_ set, neither in the rseq c.s. descriptor nor in the TLS @flags,
it's pretty much straightforward: upon signal delivery, the kernel moves the
ip to abort, and clears the tls @rseq_cs pointer. This means that any system
call issued within the signal handler is not actually within the rseq c.s.
upon which the signal is nested.
The case I worry about is if a thread sets the RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
flag in its TLS @flags field (useful in a debugging scenario where we want a
debugger to single-step through the rseq c.s. and observe registers at each step).
Arguably, this is only ever used in development. However, it does allow a situation
where a system call executed within a signal handler can nest over a rseq c.s..
So if we choose to be very strict and SIGSEGV any syscall nested over rseq
c.s., we may very well end up killing the process for no good reason in this
scenario.
- We need to decide whether all syscalls are disallowed, or if we want to pick
specific ones (e.g. fork()).
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Peter Zijlstra @ 2018-03-29 14:23 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Thomas Gleixner, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <87410797.545.1522331641598.JavaMail.zimbra@efficios.com>
On Thu, Mar 29, 2018 at 09:54:01AM -0400, Mathieu Desnoyers wrote:
> Let's say we disallow system calls from rseq critical sections. A few points
> arise:
>
> - We still need to allow traps (page faults, breakpoints, ...) within rseq c.s.,
>
> - We still need to allow interrupts within rseq c.s.,
Sure, but all those are different entry points, so that shouldn't be a
problem.
> - We need to decide whether we just document that syscalls within rseq c.s.
> are not supported, or we enforce a behavior if this happens (e.g. SIGSEGV).
> If we enforce a SIGSEGV, we'd have to figure out whether it's worth it to
> add extra branches to the system call fast path to validate this.
Without enforcement someone will eventually do this :/ We might (maybe)
get away with it being a debug option somewhere, but even that sounds
like trouble.
> - We need to carefully consider the case of system calls issued within signal
> handlers nested on top of rseq. When RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL is
> _not_ set, neither in the rseq c.s. descriptor nor in the TLS @flags,
> it's pretty much straightforward: upon signal delivery, the kernel moves the
> ip to abort, and clears the tls @rseq_cs pointer. This means that any system
> call issued within the signal handler is not actually within the rseq c.s.
> upon which the signal is nested.
>
> The case I worry about is if a thread sets the RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
> flag in its TLS @flags field (useful in a debugging scenario where we want a
> debugger to single-step through the rseq c.s. and observe registers at each step).
> Arguably, this is only ever used in development. However, it does allow a situation
> where a system call executed within a signal handler can nest over a rseq c.s..
> So if we choose to be very strict and SIGSEGV any syscall nested over rseq
> c.s., we may very well end up killing the process for no good reason in this
> scenario.
Yes, that needs a little thought; but when we run the signal handler,
the IP would no longer be inside the active RSEQ, right?
> - We need to decide whether all syscalls are disallowed, or if we want to pick
> specific ones (e.g. fork()).
All.
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Al Viro @ 2018-03-29 14:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329085305.GA22215@lst.de>
On Thu, Mar 29, 2018 at 10:53:05AM +0200, Christoph Hellwig wrote:
> On Wed, Mar 28, 2018 at 05:35:26PM +0100, Al Viro wrote:
> > > ret = vfs_fsync(req->file, req->datasync);
> > > - fput(req->file);
> > > - aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> > > + if (aio_complete(iocb, ret, 0, 0))
> > > + fput(file);
> >
> > IDGI.
> > 1) can aio_complete() ever return false here?
>
> It won't. But sometimes checking the return value and sometimes not
> seems like a bad pattern.
>
> > 2) do we ever have aio_kiocb that would not have an associated
> > struct file * that needs to be dropped on successful aio_complete()? AFAICS,
> > rw, fsync and poll variants all have one, and I'm not sure what kind of
> > async IO *could* be done without an opened file.
>
> All have a file assoiated at least right now. As mentioned last time
> finding a struct to pass that file would be rather annoying, so we'd either
> have to pass it explicitly, or do something nasty like duplicating the
> pointer in the aio_kiocb in addition to struct kiocb. Which might not
> be that bad after all, as it would only bloat the aio_kiocb and not
> struct kiocb used on stack all over.
OK. Let's leave that alone for now. Re deferred cancels - AFAICS, we *must*
remove the sucker from ctx->active_reqs before dropping ->ctx_lock.
As it is, you are creating a io_cancel()/io_cancel() race leading to double
fput(). It's not that hard to fix; I can do that myself while applying your
series (as described in previous posting - kiocb_cancel_locked() returning
NULL or ERR_PTR() in non-deferred case and pointer to aio_kiocb removed from
->active_reqs in deferred one) or you could fix it in some other way and
update your branch.
As it is, the race is user-exploitable and not that hard to trigger - AIO_POLL,
then have two threads try and cancel it at the same time.
--
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 for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-29 15:39 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas, Will Deacon <will>
In-Reply-To: <20180329142338.GD4043@hirez.programming.kicks-ass.net>
----- On Mar 29, 2018, at 10:23 AM, Peter Zijlstra peterz@infradead.org wrote:
> On Thu, Mar 29, 2018 at 09:54:01AM -0400, Mathieu Desnoyers wrote:
>> Let's say we disallow system calls from rseq critical sections. A few points
>> arise:
>>
>> - We still need to allow traps (page faults, breakpoints, ...) within rseq c.s.,
>>
>> - We still need to allow interrupts within rseq c.s.,
>
> Sure, but all those are different entry points, so that shouldn't be a
> problem.
Yes, indeed.
>
>> - We need to decide whether we just document that syscalls within rseq c.s.
>> are not supported, or we enforce a behavior if this happens (e.g. SIGSEGV).
>> If we enforce a SIGSEGV, we'd have to figure out whether it's worth it to
>> add extra branches to the system call fast path to validate this.
>
> Without enforcement someone will eventually do this :/ We might (maybe)
> get away with it being a debug option somewhere, but even that sounds
> like trouble.
I find it unlikely that someone will issue a syscall from a rseq critical
section without really intending it. The system call would need to be
crafted within the rseq assembly block.
Enforcing SIGSEGV on syscall entry when nested in a rseq critical section
will not be free both in terms of syscall overhead, and in terms of code
maintenance: we'd need to add those checks into entry.S for each architecture
supported, which pretty much doubles the amount of architecture-specific
code we need to implement for rseq. Currently, all we need is to hook in
signal delivery and wire up the system call numbers.
If there is some clever arch-agnostic way to enforce SIGSEGV in those
situations, I'm all ears. But I don't think it's worthwhile to enforce
this if it costs in terms of system call speed and adds extra arch-specific
code to maintain.
We could simply document that issuing a system call within a rseq critical
section will cause the restart behavior (whether the critical section is
restarted or not) to be undefined.
>
>> - We need to carefully consider the case of system calls issued within signal
>> handlers nested on top of rseq. When RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL is
>> _not_ set, neither in the rseq c.s. descriptor nor in the TLS @flags,
>> it's pretty much straightforward: upon signal delivery, the kernel moves the
>> ip to abort, and clears the tls @rseq_cs pointer. This means that any system
>> call issued within the signal handler is not actually within the rseq c.s.
>> upon which the signal is nested.
>>
>> The case I worry about is if a thread sets the RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
>> flag in its TLS @flags field (useful in a debugging scenario where we want a
>> debugger to single-step through the rseq c.s. and observe registers at each
>> step).
>> Arguably, this is only ever used in development. However, it does allow a
>> situation
>> where a system call executed within a signal handler can nest over a rseq c.s..
>> So if we choose to be very strict and SIGSEGV any syscall nested over rseq
>> c.s., we may very well end up killing the process for no good reason in this
>> scenario.
>
> Yes, that needs a little thought; but when we run the signal handler,
> the IP would no longer be inside the active RSEQ, right?
Good point, I missed that. So yes, even with the RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
flag set, the instruction pointer comparison would detect that we're not actually
running in the rseq critical section if a syscall is issued from the signal handler.
>
>> - We need to decide whether all syscalls are disallowed, or if we want to pick
>> specific ones (e.g. fork()).
>
> All.
I'm fine with that.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Steven Rostedt @ 2018-03-29 16:24 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Thomas Gleixner, Paul E. McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andrew Hunter, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <544124089.623.1522337940950.JavaMail.zimbra@efficios.com>
On Thu, 29 Mar 2018 11:39:00 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> Enforcing SIGSEGV on syscall entry when nested in a rseq critical section
> will not be free both in terms of syscall overhead, and in terms of code
> maintenance: we'd need to add those checks into entry.S for each architecture
> supported, which pretty much doubles the amount of architecture-specific
> code we need to implement for rseq. Currently, all we need is to hook in
> signal delivery and wire up the system call numbers.
Why not have the check on syscall exit? Then we could use the ptrace
type mechanism to only go that path when a rseq exists for the program.
-- Steve
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-29 18:02 UTC (permalink / raw)
To: rostedt
Cc: Peter Zijlstra, Thomas Gleixner, Paul E. McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andrew Hunter, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <20180329122439.4a909c72@gandalf.local.home>
----- On Mar 29, 2018, at 12:24 PM, rostedt rostedt@goodmis.org wrote:
> On Thu, 29 Mar 2018 11:39:00 -0400 (EDT)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> Enforcing SIGSEGV on syscall entry when nested in a rseq critical section
>> will not be free both in terms of syscall overhead, and in terms of code
>> maintenance: we'd need to add those checks into entry.S for each architecture
>> supported, which pretty much doubles the amount of architecture-specific
>> code we need to implement for rseq. Currently, all we need is to hook in
>> signal delivery and wire up the system call numbers.
>
> Why not have the check on syscall exit? Then we could use the ptrace
> type mechanism to only go that path when a rseq exists for the program.
Currently, anyone using ptrace on a process has pretty much given up all
hopes of performance. Processes will use rseq to gain performance, not the
opposite, so this deterioration will be unwelcome.
One thing I would find more acceptable is to only compile in this check under
a CONFIG_DEBUG_RSEQ option or something similar. This means we can then put
the check at the most convenient location without caring too much about its
performance impact.
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Manfred Spraul @ 2018-03-29 18:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Davidlohr Bueso, Stanislav Kinsbursky, Kees Cook,
linux-api-u79uwXL29TY76Z2rM5mHXA, Linux Containers,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Al Viro, Luis R. Rodriguez,
Michael Kerrisk, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Waiman Long, Andrew Morton, Eric W. Biederman
In-Reply-To: <20180329105601.GA597-PfSpb0PWhxZc2C7mugBRk2EX/6BAtgUQ@public.gmane.org>
Hello Mathew,
On 03/29/2018 12:56 PM, Matthew Wilcox wrote:
> On Thu, Mar 29, 2018 at 10:47:45AM +0200, Manfred Spraul wrote:
>>>>>>> This can be implemented trivially with the current code
>>>>>>> using idr_alloc_cyclic.
>> Is there a performance impact?
>> Right now, the idr tree is only large if there are lots of objects.
>> What happens if we have only 1 object, with id=INT_MAX-1?
> The radix tree uses a branching factor of 64 entries (6 bits) per level.
> The maximum ID is 31 bits (positive signed 32-bit integer). So the
> worst case for a single object is 6 pointer dereferences to find the
> object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12
> cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2
> (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.
I'm concerned about the up to 6 branches.
But this is just guessing, we need a test with a realistic workload.
--
Manfred
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Steven Rostedt @ 2018-03-29 18:07 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Peter Zijlstra, Thomas Gleixner, Paul E. McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andrew Hunter, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <21903915.856.1522346553810.JavaMail.zimbra@efficios.com>
On Thu, 29 Mar 2018 14:02:33 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> Currently, anyone using ptrace on a process has pretty much given up all
> hopes of performance. Processes will use rseq to gain performance, not the
> opposite, so this deterioration will be unwelcome.
The ptrace path has nothing to do with ptrace anymore, and probably be
hard to notice the performance hit. You simply set a TIF flag, and on
exit of the syscall it jumps to a path that checks special cases
(tracing system calls being one of them). It's called the ptrace path
because ptrace was the first one to use it (I'm guessing, I haven't
actually looked at the history).
This is used to add any system call checks that are not done during
normal operation. And this certainly falls under that category.
-- Steve
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Christoph Hellwig @ 2018-03-29 18:08 UTC (permalink / raw)
To: Al Viro
Cc: Christoph Hellwig, Avi Kivity, linux-aio, linux-fsdevel, netdev,
linux-api, linux-kernel
In-Reply-To: <20180329142506.GX30522@ZenIV.linux.org.uk>
On Thu, Mar 29, 2018 at 03:25:06PM +0100, Al Viro wrote:
> OK. Let's leave that alone for now. Re deferred cancels - AFAICS, we *must*
> remove the sucker from ctx->active_reqs before dropping ->ctx_lock.
>
> As it is, you are creating a io_cancel()/io_cancel() race leading to double
> fput(). It's not that hard to fix; I can do that myself while applying your
> series (as described in previous posting - kiocb_cancel_locked() returning
> NULL or ERR_PTR() in non-deferred case and pointer to aio_kiocb removed from
> ->active_reqs in deferred one) or you could fix it in some other way and
> update your branch.
I think that is the right fix. Let me resend so that I can test the
result first.
--
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 for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Mathieu Desnoyers @ 2018-03-29 18:35 UTC (permalink / raw)
To: rostedt
Cc: Peter Zijlstra, Thomas Gleixner, Paul E. McKenney, Boqun Feng,
Andy Lutomirski, Dave Watson, linux-kernel, linux-api,
Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andrew Hunter, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <20180329140752.12661138@gandalf.local.home>
----- On Mar 29, 2018, at 2:07 PM, rostedt rostedt@goodmis.org wrote:
> On Thu, 29 Mar 2018 14:02:33 -0400 (EDT)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>> Currently, anyone using ptrace on a process has pretty much given up all
>> hopes of performance. Processes will use rseq to gain performance, not the
>> opposite, so this deterioration will be unwelcome.
>
> The ptrace path has nothing to do with ptrace anymore, and probably be
> hard to notice the performance hit. You simply set a TIF flag, and on
> exit of the syscall it jumps to a path that checks special cases
> (tracing system calls being one of them). It's called the ptrace path
> because ptrace was the first one to use it (I'm guessing, I haven't
> actually looked at the history).
Last time I checked, it's not only a jump, it's actually saving/restoring
tons of registers. Did this change recently ?
I use it for LTTng syscall tracing too. My experience so far is that it's really
terribly slow. I've been waiting on Andy Lutomirski to complete his changes in that
area to look into making this faster for syscall tracepoints.
>
> This is used to add any system call checks that are not done during
> normal operation. And this certainly falls under that category.
I know it's used for stuff like seccomp too. My guess has always been that security
people care much more about robustness than performance.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [REVIEW][PATCH 00/11] ipc: Fixing the pid namespace support
From: Eric W. Biederman @ 2018-03-29 18:42 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Linux Containers, linux-kernel, linux-api, khlebnikov,
prakash.sangappa, luto, akpm, oleg, serge.hallyn, esyr, jannh,
linux-security-module, Pavel Emelyanov, Nagarathnam Muthusamy
In-Reply-To: <20180329011241.v5kgiwbbayz425hk@linux-n805>
Davidlohr Bueso <dave@stgolabs.net> writes:
> On Fri, 23 Mar 2018, Eric W. Biederman wrote:
>
>>Still I would like to see this fixed and I plan on merging this code.
The code is merged into my for-next tree now.
> Yes, it needs fixed, but 1) there are pending issues (such as the
> extra atomics)
Concerns not issues. I documented them but I don't see any serious
reason to be concerned. The data structures are sufficiently different
from AF_UNIX as well as the usage patterns that I have no reasonable
expectation that there will be problems.
There is no reasonable alternate implementation for correcting this bug.
Because of my concerns I looked at several other possibilities and they
all showed incorrect behavior, in different circumstances.
The implementations are simple enough there are no deep subtle issues.
I have tested the code.
If a regression happens the code is carefully split up so things can be
bisected easily and reverted if necessary.
> and 2) its late in the -rc cycle. Plus this issue has existed for 11 years without
> the world ending, so I'm sure we can hold on until at least one more
> release.
People really are starting to seriously look at accessing a single ipc
namespace from multiple pid namespaces. The work arounds I saw posted
for the current brokenness were too nasty to live.
Better to fix things before there is code that actually starts depending
on the current brokenness.
I am the namespace maintianer and this is my area of responsibility.
The code is ready and I see no reason or benefit in delay.
Eric
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Steven Rostedt @ 2018-03-29 18:46 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Paul E. McKenney, Boqun Feng, Dave Watson, linux-kernel,
linux-api, Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andrew Hunter, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <1657598086.875.1522348516598.JavaMail.zimbra@efficios.com>
On Thu, 29 Mar 2018 14:35:16 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- On Mar 29, 2018, at 2:07 PM, rostedt rostedt@goodmis.org wrote:
>
> > On Thu, 29 Mar 2018 14:02:33 -0400 (EDT)
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >
> >> Currently, anyone using ptrace on a process has pretty much given up all
> >> hopes of performance. Processes will use rseq to gain performance, not the
> >> opposite, so this deterioration will be unwelcome.
> >
> > The ptrace path has nothing to do with ptrace anymore, and probably be
> > hard to notice the performance hit. You simply set a TIF flag, and on
> > exit of the syscall it jumps to a path that checks special cases
> > (tracing system calls being one of them). It's called the ptrace path
> > because ptrace was the first one to use it (I'm guessing, I haven't
> > actually looked at the history).
>
> Last time I checked, it's not only a jump, it's actually saving/restoring
> tons of registers. Did this change recently ?
>
> I use it for LTTng syscall tracing too. My experience so far is that it's really
> terribly slow. I've been waiting on Andy Lutomirski to complete his changes in that
> area to look into making this faster for syscall tracepoints.
This gives us more incentive to help Andy make it faster ;-)
-- Steve
>
> >
> > This is used to add any system call checks that are not done during
> > normal operation. And this certainly falls under that category.
>
> I know it's used for stuff like seccomp too. My guess has always been that security
> people care much more about robustness than performance.
>
> Thanks,
>
> Mathieu
>
>
^ permalink raw reply
* Re: [RFC PATCH for 4.17 02/21] rseq: Introduce restartable sequences system call (v12)
From: Steven Rostedt @ 2018-03-29 18:47 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Paul E. McKenney, Boqun Feng, Dave Watson, linux-kernel,
linux-api, Paul Turner, Andrew Morton, Russell King, Ingo Molnar,
H. Peter Anvin, Andrew Hunter, Andi Kleen, Chris Lameter,
Ben Maurer, Josh Triplett, Linus Torvalds, Catalin Marinas
In-Reply-To: <20180329144605.599a8f46@gandalf.local.home>
On Thu, 29 Mar 2018 14:46:05 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> This gives us more incentive to help Andy make it faster ;-)
And with Meltdown, I doubt this makes as big of a difference anymore :-/
-- Steve
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Eric W. Biederman @ 2018-03-29 18:52 UTC (permalink / raw)
To: Manfred Spraul
Cc: Matthew Wilcox, Davidlohr Bueso, Waiman Long, Michael Kerrisk,
Luis R. Rodriguez, Kees Cook, linux-kernel, linux-fsdevel,
Andrew Morton, Al Viro, Stanislav Kinsbursky, Linux Containers,
linux-api
In-Reply-To: <05772f83-d680-aea1-b222-cef2430dcc83@colorfullife.com>
Manfred Spraul <manfred@colorfullife.com> writes:
> Hello Mathew,
>
> On 03/29/2018 12:56 PM, Matthew Wilcox wrote:
>> On Thu, Mar 29, 2018 at 10:47:45AM +0200, Manfred Spraul wrote:
>>>>>>>> This can be implemented trivially with the current code
>>>>>>>> using idr_alloc_cyclic.
>>> Is there a performance impact?
>>> Right now, the idr tree is only large if there are lots of objects.
>>> What happens if we have only 1 object, with id=INT_MAX-1?
>> The radix tree uses a branching factor of 64 entries (6 bits) per level.
>> The maximum ID is 31 bits (positive signed 32-bit integer). So the
>> worst case for a single object is 6 pointer dereferences to find the
>> object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12
>> cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2
>> (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.
> I'm concerned about the up to 6 branches.
> But this is just guessing, we need a test with a realistic workload.
Yes.
My primary purpose with the patch was to show that the issues with the
current limits could be resolved in a straght forward manner. I really
don't know if idrs are the appropriate data structure. It is possible
rbtrees are a better fit.
I think my algorithm I proposed for generating identifiers is as likely
as any to be a good one. It does need testing on a wide variety of
applications to see what applications actually care about and for that I
think my proposed patch is more than sufficient.
By not keeping a generation counters in the slots themselves linux
already differs substantially from traditional implementations.
Doing something to free us from using a fixed number of bits for the
counter and a fixed number of bits to encode the slot we can support
much larger use of this API.
Eric
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Matthew Wilcox @ 2018-03-29 19:32 UTC (permalink / raw)
To: Manfred Spraul
Cc: Davidlohr Bueso, Waiman Long, Michael Kerrisk, Eric W. Biederman,
Luis R. Rodriguez, Kees Cook, linux-kernel, linux-fsdevel,
Andrew Morton, Al Viro, Stanislav Kinsbursky, Linux Containers,
linux-api
In-Reply-To: <05772f83-d680-aea1-b222-cef2430dcc83@colorfullife.com>
On Thu, Mar 29, 2018 at 08:07:44PM +0200, Manfred Spraul wrote:
> Hello Mathew,
>
> On 03/29/2018 12:56 PM, Matthew Wilcox wrote:
> > On Thu, Mar 29, 2018 at 10:47:45AM +0200, Manfred Spraul wrote:
> > > > > > > > This can be implemented trivially with the current code
> > > > > > > > using idr_alloc_cyclic.
> > > Is there a performance impact?
> > > Right now, the idr tree is only large if there are lots of objects.
> > > What happens if we have only 1 object, with id=INT_MAX-1?
> > The radix tree uses a branching factor of 64 entries (6 bits) per level.
> > The maximum ID is 31 bits (positive signed 32-bit integer). So the
> > worst case for a single object is 6 pointer dereferences to find the
> > object anywhere in the range (INT_MAX/2 - INT_MAX]. That will read 12
> > cachelines. If we were to constrain ourselves to a maximum of INT_MAX/2
> > (30 bits), we'd reduce that to 5 pointer dereferences and 10 cachelines.
> I'm concerned about the up to 6 branches.
> But this is just guessing, we need a test with a realistic workload.
Yes, and once there's a realistic workload, I'll be happy to prioritise
adapting the data structure to reduce the pointer chases.
FWIW, the plan is this:
There's currently an unused 32-bit field (on 64-bit machines) which I plan
to make the 'base' field. So at each step down the tree, one subtracts
that field from the index in order to decide which slot to look at next.
Something like this:
index=0x3000'0000
(root) -> order=24
offset=48 -> order=18
offset=0 -> order=12
offset=0 -> order=6
offset=0 -> order=0
offset=0 -> data
compresses to a single node:
(root) -> order=0
base=3000'0000
offset=0 -> data
If one has one entry at 5 and another entry at 0x3000'0000, the tree
looks like this (three nodes):
(root) -> order=24
base=0
offset=0 -> order=0
base=0
offset=5 -> entry1
offset=48 -> order=0
base=0
offset=0 -> entry2
The trick is making sure that looking up offset 0x300'1000 returns NULL
and not entry2, but I can make that work.
An alternative is to go to something a little more libJudy and have
mutating internal nodes in the tree that can represent this kind of
situation in a more compact form. There's a tradeoff to be made between
simplicity of implementation, cost of insertion, cost of lookup and
memory consumption. I don't know where the right balance point is yet.
^ permalink raw reply
* Re: [RFC][PATCH] ipc: Remove IPCMNI
From: Eric W. Biederman @ 2018-03-29 20:08 UTC (permalink / raw)
To: Manfred Spraul
Cc: Davidlohr Bueso, Waiman Long, Michael Kerrisk, Luis R. Rodriguez,
Kees Cook, linux-kernel, linux-fsdevel, Andrew Morton, Al Viro,
Matthew Wilcox, Stanislav Kinsbursky, Linux Containers, linux-api
In-Reply-To: <3e201de2-bed2-6f7d-0783-700d095142e0@colorfullife.com>
Manfred Spraul <manfred@colorfullife.com> writes:
>>>>> On 03/14/2018 08:49 PM, Eric W. Biederman wrote:
>>>>>> To make it possible to keep checkpoint/restore working I have renamed
>>>>>> the sysctls from xxx_next_id to xxx_nextid. That is enough change that
>>>>>> a smart CRIU implementation can see that what is exported has changed,
>>>>>> and act accordingly. New kernels will be able to restore the old id's.
>>>>>>
>>>>>> This code still needs some real world testing to verify my assumptions.
>>>>>> And some work with the CRIU implementations to actually add the code
>>>>>> that deals with the new for of id assignment.
>>>>>>
> It means that all existing checkpoint/restore application will not work with a
> new kernel.
> Everyone must first update the checkpoint/restore application, then update the
> kernel.
>
> Is this acceptable?
There is no nead.
I just reread through how next_id is implementated in ipc/util.c and I
had been reading it wrong. There is no need to change the sysctl.
What criu needs is an interface that specifies the next_id to allocate
both the high and the low bits and that is what this the sysctl
provides.
The implemenation could use a little cleanup so it is easier to
understand something like this perhaps:
diff --git a/ipc/util.c b/ipc/util.c
index 3783b7991cc7..2d4ec6e5b70b 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -192,46 +192,32 @@ static struct kern_ipc_perm *ipc_findkey(struct ipc_ids *ids, key_t key)
return NULL;
}
-#ifdef CONFIG_CHECKPOINT_RESTORE
-/*
- * Specify desired id for next allocated IPC object.
- */
-#define ipc_idr_alloc(ids, new) \
- idr_alloc(&(ids)->ipcs_idr, (new), \
- (ids)->next_id < 0 ? 0 : ipcid_to_idx((ids)->next_id),\
- 0, GFP_NOWAIT)
-
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
- struct kern_ipc_perm *new)
-{
- if (ids->next_id < 0) { /* default, behave as !CHECKPOINT_RESTORE */
- new->seq = ids->seq++;
- if (ids->seq > IPCID_SEQ_MAX)
- ids->seq = 0;
- } else {
- new->seq = ipcid_to_seqx(ids->next_id);
- ids->next_id = -1;
- }
- return SEQ_MULTIPLIER * new->seq + id;
-}
-
-#else
-#define ipc_idr_alloc(ids, new) \
- idr_alloc(&(ids)->ipcs_idr, (new), 0, 0, GFP_NOWAIT)
-
-static inline int ipc_buildid(int id, struct ipc_ids *ids,
- struct kern_ipc_perm *new)
+static inline int ipc_idr_alloc(struct ipc_ids *ids, struct kern_ipc_perm *new)
{
+ int id;
+#ifdef CONFIG_CHECKPOINT_RESTORE
+ if (unlikely(new->id >= 0)) {
+ int idx = ipcid_to_idx(new->id);
+ id = idr_alloc(&ids->ipcs_idr, new, idx, 0, GFP_NOWAIT);
+ if (id < 0)
+ return id;
+ if (id != idx) {
+ idr_remove(&ids->ipcs_idr, id);
+ return -EBUSY;
+ }
+ new->seq = ipcid_to_seqx(new->id);
+ return id;
+ }
+#endif
+ id = idr_alloc(&ids->ipcs_idr, new, 0, 0, GFP_NOWAIT);
new->seq = ids->seq++;
if (ids->seq > IPCID_SEQ_MAX)
ids->seq = 0;
-
- return SEQ_MULTIPLIER * new->seq + id;
+ new->id = SEQ_MULTIPLIER * new->seq + id;
+ return id;
}
-#endif /* CONFIG_CHECKPOINT_RESTORE */
-
/**
* ipc_addid - add an ipc identifier
* @ids: ipc identifier set
@@ -269,6 +255,10 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
new->cuid = new->uid = euid;
new->gid = new->cgid = egid;
+ new->id = ids->next_id;
+ if (new->id >= 0)
+ ids->next_id = -1;
+
id = ipc_idr_alloc(ids, new);
idr_preload_end();
@@ -290,8 +280,6 @@ int ipc_addid(struct ipc_ids *ids, struct kern_ipc_perm *new, int limit)
if (id > ids->max_id)
ids->max_id = id;
- new->id = ipc_buildid(id, ids, new);
-
return id;
}
Eric
^ permalink raw reply related
* aio poll and a new in-kernel poll API V8
From: Christoph Hellwig @ 2018-03-29 20:32 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
Hi all,
this series adds support for the IOCB_CMD_POLL operation to poll for the
readyness of file descriptors using the aio subsystem. The API is based
on patches that existed in RHAS2.1 and RHEL3, which means it already is
supported by libaio. To implement the poll support efficiently new
methods to poll are introduced in struct file_operations: get_poll_head
and poll_mask. The first one returns a wait_queue_head to wait on
(lifetime is bound by the file), and the second does a non-blocking
check for the POLL* events. This allows aio poll to work without
any additional context switches, unlike epoll.
This series sits on top of the aio-fsync series that also includes
support for io_pgetevents.
The changes were sponsored by Scylladb, and improve performance
of the seastar framework up to 10%, while also removing the need
for a privileged SCHED_FIFO epoll listener thread.
git://git.infradead.org/users/hch/vfs.git aio-poll.8
Gitweb:
http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/aio-poll.8
Libaio changes:
https://pagure.io/libaio.git io-poll
Seastar changes (not updated for the new io_pgetevens ABI yet):
https://github.com/avikivity/seastar/commits/aio
Changes since V8:
- make delayed cancellation safe and unconditional
Changes since V7:
- reworked cancellation
Changes since V6:
- small changelog updates
- rebased on top of the aio-fsync changes
Changes since V4:
- rebased ontop of Linux 4.16-rc4
Changes since V3:
- remove the pre-sleep ->poll_mask call in vfs_poll,
allow ->get_poll_head to return POLL* values.
Changes since V2:
- removed a double initialization
- new vfs_get_poll_head helper
- document that ->get_poll_head can return NULL
- call ->poll_mask before sleeping
- various ACKs
- add conversion of random to ->poll_mask
- add conversion of af_alg to ->poll_mask
- lacking ->poll_mask support now returns -EINVAL for IOCB_CMD_POLL
- reshuffled the series so that prep patches and everything not
requiring the new in-kernel poll API is in the beginning
Changes since V1:
- handle the NULL ->poll case in vfs_poll
- dropped the file argument to the ->poll_mask socket operation
- replace the ->pre_poll socket operation with ->get_poll_head as
in the file operations
--
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
* [PATCH 01/30] fs: unexport poll_schedule_timeout
From: Christoph Hellwig @ 2018-03-29 20:32 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
No users outside of select.c.
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/select.c | 3 +--
include/linux/poll.h | 2 --
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/select.c b/fs/select.c
index b6c36254028a..686de7b3a1db 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -233,7 +233,7 @@ static void __pollwait(struct file *filp, wait_queue_head_t *wait_address,
add_wait_queue(wait_address, &entry->wait);
}
-int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
+static int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
ktime_t *expires, unsigned long slack)
{
int rc = -EINTR;
@@ -258,7 +258,6 @@ int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
return rc;
}
-EXPORT_SYMBOL(poll_schedule_timeout);
/**
* poll_select_set_timeout - helper function to setup the timeout value
diff --git a/include/linux/poll.h b/include/linux/poll.h
index f45ebd017eaa..a3576da63377 100644
--- a/include/linux/poll.h
+++ b/include/linux/poll.h
@@ -96,8 +96,6 @@ struct poll_wqueues {
extern void poll_initwait(struct poll_wqueues *pwq);
extern void poll_freewait(struct poll_wqueues *pwq);
-extern int poll_schedule_timeout(struct poll_wqueues *pwq, int state,
- ktime_t *expires, unsigned long slack);
extern u64 select_estimate_accuracy(struct timespec64 *tv);
#define MAX_INT64_SECONDS (((s64)(~((u64)0)>>1)/HZ)-1)
--
2.14.2
--
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 related
* [PATCH 02/30] fs: cleanup do_pollfd
From: Christoph Hellwig @ 2018-03-29 20:33 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
Use straightline code with failure handling gotos instead of a lot
of nested conditionals.
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/select.c | 48 +++++++++++++++++++++++-------------------------
1 file changed, 23 insertions(+), 25 deletions(-)
diff --git a/fs/select.c b/fs/select.c
index 686de7b3a1db..c6c504a814f9 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -806,34 +806,32 @@ static inline __poll_t do_pollfd(struct pollfd *pollfd, poll_table *pwait,
bool *can_busy_poll,
__poll_t busy_flag)
{
- __poll_t mask;
- int fd;
-
- mask = 0;
- fd = pollfd->fd;
- if (fd >= 0) {
- struct fd f = fdget(fd);
- mask = EPOLLNVAL;
- if (f.file) {
- /* userland u16 ->events contains POLL... bitmap */
- __poll_t filter = demangle_poll(pollfd->events) |
- EPOLLERR | EPOLLHUP;
- mask = DEFAULT_POLLMASK;
- if (f.file->f_op->poll) {
- pwait->_key = filter;
- pwait->_key |= busy_flag;
- mask = f.file->f_op->poll(f.file, pwait);
- if (mask & busy_flag)
- *can_busy_poll = true;
- }
- /* Mask out unneeded events. */
- mask &= filter;
- fdput(f);
- }
+ int fd = pollfd->fd;
+ __poll_t mask = 0, filter;
+ struct fd f;
+
+ if (fd < 0)
+ goto out;
+ mask = EPOLLNVAL;
+ f = fdget(fd);
+ if (!f.file)
+ goto out;
+
+ /* userland u16 ->events contains POLL... bitmap */
+ filter = demangle_poll(pollfd->events) | EPOLLERR | EPOLLHUP;
+ mask = DEFAULT_POLLMASK;
+ if (f.file->f_op->poll) {
+ pwait->_key = filter | busy_flag;
+ mask = f.file->f_op->poll(f.file, pwait);
+ if (mask & busy_flag)
+ *can_busy_poll = true;
}
+ mask &= filter; /* Mask out unneeded events. */
+ fdput(f);
+
+out:
/* ... and so does ->revents */
pollfd->revents = mangle_poll(mask);
-
return mask;
}
--
2.14.2
--
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 related
* [PATCH 03/30] fs: update documentation to mention __poll_t and match the code
From: Christoph Hellwig @ 2018-03-29 20:33 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/filesystems/Locking | 2 +-
Documentation/filesystems/vfs.txt | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 75d2d57e2c44..220bba28f72b 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -439,7 +439,7 @@ prototypes:
ssize_t (*read_iter) (struct kiocb *, struct iov_iter *);
ssize_t (*write_iter) (struct kiocb *, struct iov_iter *);
int (*iterate) (struct file *, struct dir_context *);
- unsigned int (*poll) (struct file *, struct poll_table_struct *);
+ __poll_t (*poll) (struct file *, struct poll_table_struct *);
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index 5fd325df59e2..f608180ad59d 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -856,7 +856,7 @@ struct file_operations {
ssize_t (*read_iter) (struct kiocb *, struct iov_iter *);
ssize_t (*write_iter) (struct kiocb *, struct iov_iter *);
int (*iterate) (struct file *, struct dir_context *);
- unsigned int (*poll) (struct file *, struct poll_table_struct *);
+ __poll_t (*poll) (struct file *, struct poll_table_struct *);
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
--
2.14.2
--
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 related
* [PATCH 04/30] fs: add new vfs_poll and file_can_poll helpers
From: Christoph Hellwig @ 2018-03-29 20:33 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
These abstract out calls to the poll method in preparation for changes
in how we poll.
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>
---
drivers/staging/comedi/drivers/serial2002.c | 4 ++--
drivers/vfio/virqfd.c | 2 +-
drivers/vhost/vhost.c | 2 +-
fs/eventpoll.c | 5 ++---
fs/select.c | 23 ++++++++---------------
include/linux/poll.h | 12 ++++++++++++
mm/memcontrol.c | 2 +-
net/9p/trans_fd.c | 18 ++++--------------
virt/kvm/eventfd.c | 2 +-
9 files changed, 32 insertions(+), 38 deletions(-)
diff --git a/drivers/staging/comedi/drivers/serial2002.c b/drivers/staging/comedi/drivers/serial2002.c
index b3f3b4a201af..5471b2212a62 100644
--- a/drivers/staging/comedi/drivers/serial2002.c
+++ b/drivers/staging/comedi/drivers/serial2002.c
@@ -113,7 +113,7 @@ static void serial2002_tty_read_poll_wait(struct file *f, int timeout)
long elapsed;
__poll_t mask;
- mask = f->f_op->poll(f, &table.pt);
+ mask = vfs_poll(f, &table.pt);
if (mask & (EPOLLRDNORM | EPOLLRDBAND | EPOLLIN |
EPOLLHUP | EPOLLERR)) {
break;
@@ -136,7 +136,7 @@ static int serial2002_tty_read(struct file *f, int timeout)
result = -1;
if (!IS_ERR(f)) {
- if (f->f_op->poll) {
+ if (file_can_poll(f)) {
serial2002_tty_read_poll_wait(f, timeout);
if (kernel_read(f, &ch, 1, &pos) == 1)
diff --git a/drivers/vfio/virqfd.c b/drivers/vfio/virqfd.c
index 085700f1be10..2a1be859ee71 100644
--- a/drivers/vfio/virqfd.c
+++ b/drivers/vfio/virqfd.c
@@ -166,7 +166,7 @@ int vfio_virqfd_enable(void *opaque,
init_waitqueue_func_entry(&virqfd->wait, virqfd_wakeup);
init_poll_funcptr(&virqfd->pt, virqfd_ptable_queue_proc);
- events = irqfd.file->f_op->poll(irqfd.file, &virqfd->pt);
+ events = vfs_poll(irqfd.file, &virqfd->pt);
/*
* Check if there was an event already pending on the eventfd
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 1b3e8d2d5c8b..4d27e288bb1d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -208,7 +208,7 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
if (poll->wqh)
return 0;
- mask = file->f_op->poll(file, &poll->table);
+ mask = vfs_poll(file, &poll->table);
if (mask)
vhost_poll_wakeup(&poll->wait, 0, 0, poll_to_key(mask));
if (mask & EPOLLERR) {
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 0f3494ed3ed0..2bebae5a38cf 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -884,8 +884,7 @@ static __poll_t ep_item_poll(const struct epitem *epi, poll_table *pt,
pt->_key = epi->event.events;
if (!is_file_epoll(epi->ffd.file))
- return epi->ffd.file->f_op->poll(epi->ffd.file, pt) &
- epi->event.events;
+ return vfs_poll(epi->ffd.file, pt) & epi->event.events;
ep = epi->ffd.file->private_data;
poll_wait(epi->ffd.file, &ep->poll_wait, pt);
@@ -2020,7 +2019,7 @@ SYSCALL_DEFINE4(epoll_ctl, int, epfd, int, op, int, fd,
/* The target file descriptor must support poll */
error = -EPERM;
- if (!tf.file->f_op->poll)
+ if (!file_can_poll(tf.file))
goto error_tgt_fput;
/* Check if EPOLLWAKEUP is allowed */
diff --git a/fs/select.c b/fs/select.c
index c6c504a814f9..ba91103707ea 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -502,14 +502,10 @@ static int do_select(int n, fd_set_bits *fds, struct timespec64 *end_time)
continue;
f = fdget(i);
if (f.file) {
- const struct file_operations *f_op;
- f_op = f.file->f_op;
- mask = DEFAULT_POLLMASK;
- if (f_op->poll) {
- wait_key_set(wait, in, out,
- bit, busy_flag);
- mask = (*f_op->poll)(f.file, wait);
- }
+ wait_key_set(wait, in, out, bit,
+ busy_flag);
+ mask = vfs_poll(f.file, wait);
+
fdput(f);
if ((mask & POLLIN_SET) && (in & bit)) {
res_in |= bit;
@@ -819,13 +815,10 @@ static inline __poll_t do_pollfd(struct pollfd *pollfd, poll_table *pwait,
/* userland u16 ->events contains POLL... bitmap */
filter = demangle_poll(pollfd->events) | EPOLLERR | EPOLLHUP;
- mask = DEFAULT_POLLMASK;
- if (f.file->f_op->poll) {
- pwait->_key = filter | busy_flag;
- mask = f.file->f_op->poll(f.file, pwait);
- if (mask & busy_flag)
- *can_busy_poll = true;
- }
+ pwait->_key = filter | busy_flag;
+ mask = vfs_poll(f.file, pwait);
+ if (mask & busy_flag)
+ *can_busy_poll = true;
mask &= filter; /* Mask out unneeded events. */
fdput(f);
diff --git a/include/linux/poll.h b/include/linux/poll.h
index a3576da63377..7e0fdcf905d2 100644
--- a/include/linux/poll.h
+++ b/include/linux/poll.h
@@ -74,6 +74,18 @@ static inline void init_poll_funcptr(poll_table *pt, poll_queue_proc qproc)
pt->_key = ~(__poll_t)0; /* all events enabled */
}
+static inline bool file_can_poll(struct file *file)
+{
+ return file->f_op->poll;
+}
+
+static inline __poll_t vfs_poll(struct file *file, struct poll_table_struct *pt)
+{
+ if (unlikely(!file->f_op->poll))
+ return DEFAULT_POLLMASK;
+ return file->f_op->poll(file, pt);
+}
+
struct poll_table_entry {
struct file *filp;
__poll_t key;
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 670e99b68aa6..8774ece5c3c3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3849,7 +3849,7 @@ static ssize_t memcg_write_event_control(struct kernfs_open_file *of,
if (ret)
goto out_put_css;
- efile.file->f_op->poll(efile.file, &event->pt);
+ vfs_poll(efile.file, &event->pt);
spin_lock(&memcg->event_list_lock);
list_add(&event->list, &memcg->event_list);
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 0cfba919d167..3811775692d0 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -231,7 +231,7 @@ static void p9_conn_cancel(struct p9_conn *m, int err)
static __poll_t
p9_fd_poll(struct p9_client *client, struct poll_table_struct *pt, int *err)
{
- __poll_t ret, n;
+ __poll_t ret;
struct p9_trans_fd *ts = NULL;
if (client && client->status == Connected)
@@ -243,19 +243,9 @@ p9_fd_poll(struct p9_client *client, struct poll_table_struct *pt, int *err)
return EPOLLERR;
}
- if (!ts->rd->f_op->poll)
- ret = DEFAULT_POLLMASK;
- else
- ret = ts->rd->f_op->poll(ts->rd, pt);
-
- if (ts->rd != ts->wr) {
- if (!ts->wr->f_op->poll)
- n = DEFAULT_POLLMASK;
- else
- n = ts->wr->f_op->poll(ts->wr, pt);
- ret = (ret & ~EPOLLOUT) | (n & ~EPOLLIN);
- }
-
+ ret = vfs_poll(ts->rd, pt);
+ if (ts->rd != ts->wr)
+ ret = (ret & ~EPOLLOUT) | (vfs_poll(ts->wr, pt) & ~EPOLLIN);
return ret;
}
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 6e865e8b5b10..90d30fbe95ae 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -397,7 +397,7 @@ kvm_irqfd_assign(struct kvm *kvm, struct kvm_irqfd *args)
* Check if there was an event already pending on the eventfd
* before we registered, and trigger it as if we didn't miss it.
*/
- events = f.file->f_op->poll(f.file, &irqfd->pt);
+ events = vfs_poll(f.file, &irqfd->pt);
if (events & EPOLLIN)
schedule_work(&irqfd->inject);
--
2.14.2
--
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 related
* [PATCH 05/30] fs: introduce new ->get_poll_head and ->poll_mask methods
From: Christoph Hellwig @ 2018-03-29 20:33 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
->get_poll_head returns the waitqueue that the poll operation is going
to sleep on. Note that this means we can only use a single waitqueue
for the poll, unlike some current drivers that use two waitqueues for
different events. But now that we have keyed wakeups and heavily use
those for poll there aren't that many good reason left to keep the
multiple waitqueues, and if there are any ->poll is still around, the
driver just won't support aio poll.
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>
---
Documentation/filesystems/Locking | 7 ++++++-
Documentation/filesystems/vfs.txt | 13 +++++++++++++
fs/select.c | 28 ++++++++++++++++++++++++++++
include/linux/fs.h | 2 ++
include/linux/poll.h | 27 +++++++++++++++++++++++----
5 files changed, 72 insertions(+), 5 deletions(-)
diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking
index 220bba28f72b..6d227f9d7bd9 100644
--- a/Documentation/filesystems/Locking
+++ b/Documentation/filesystems/Locking
@@ -440,6 +440,8 @@ prototypes:
ssize_t (*write_iter) (struct kiocb *, struct iov_iter *);
int (*iterate) (struct file *, struct dir_context *);
__poll_t (*poll) (struct file *, struct poll_table_struct *);
+ struct wait_queue_head * (*get_poll_head)(struct file *, __poll_t);
+ __poll_t (*poll_mask) (struct file *, __poll_t);
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
@@ -470,7 +472,7 @@ prototypes:
};
locking rules:
- All may block.
+ All except for ->poll_mask may block.
->llseek() locking has moved from llseek to the individual llseek
implementations. If your fs is not using generic_file_llseek, you
@@ -498,6 +500,9 @@ in sys_read() and friends.
the lease within the individual filesystem to record the result of the
operation
+->poll_mask can be called with or without the waitqueue lock for the waitqueue
+returned from ->get_poll_head.
+
--------------------------- dquot_operations -------------------------------
prototypes:
int (*write_dquot) (struct dquot *);
diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt
index f608180ad59d..50ee13563271 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -857,6 +857,8 @@ struct file_operations {
ssize_t (*write_iter) (struct kiocb *, struct iov_iter *);
int (*iterate) (struct file *, struct dir_context *);
__poll_t (*poll) (struct file *, struct poll_table_struct *);
+ struct wait_queue_head * (*get_poll_head)(struct file *, __poll_t);
+ __poll_t (*poll_mask) (struct file *, __poll_t);
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
@@ -901,6 +903,17 @@ otherwise noted.
activity on this file and (optionally) go to sleep until there
is activity. Called by the select(2) and poll(2) system calls
+ get_poll_head: Returns the struct wait_queue_head that poll, select,
+ epoll or aio poll should wait on in case this instance only has single
+ waitqueue. Can return NULL to indicate polling is not supported,
+ or a POLL* value using the POLL_TO_PTR helper in case a grave error
+ occured and ->poll_mask shall not be called.
+
+ poll_mask: return the mask of POLL* values describing the file descriptor
+ state. Called either before going to sleep on the waitqueue returned by
+ get_poll_head, or after it has been woken. If ->get_poll_head and
+ ->poll_mask are implemented ->poll does not need to be implement.
+
unlocked_ioctl: called by the ioctl(2) system call.
compat_ioctl: called by the ioctl(2) system call when 32 bit system calls
diff --git a/fs/select.c b/fs/select.c
index ba91103707ea..cc270d7f6192 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -34,6 +34,34 @@
#include <linux/uaccess.h>
+__poll_t vfs_poll(struct file *file, struct poll_table_struct *pt)
+{
+ unsigned int events = poll_requested_events(pt);
+ struct wait_queue_head *head;
+
+ if (unlikely(!file_can_poll(file)))
+ return DEFAULT_POLLMASK;
+
+ if (file->f_op->poll)
+ return file->f_op->poll(file, pt);
+
+ /*
+ * Only get the poll head and do the first mask check if we are actually
+ * going to sleep on this file:
+ */
+ if (pt && pt->_qproc) {
+ head = vfs_get_poll_head(file, events);
+ if (!head)
+ return DEFAULT_POLLMASK;
+ if (IS_ERR(head))
+ return PTR_TO_POLL(head);
+
+ pt->_qproc(file, head, pt);
+ }
+
+ return file->f_op->poll_mask(file, events);
+}
+EXPORT_SYMBOL_GPL(vfs_poll);
/*
* Estimate expected accuracy in ns from a timeval.
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 79c413985305..6ea2c0843bb1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1708,6 +1708,8 @@ struct file_operations {
int (*iterate) (struct file *, struct dir_context *);
int (*iterate_shared) (struct file *, struct dir_context *);
__poll_t (*poll) (struct file *, struct poll_table_struct *);
+ struct wait_queue_head * (*get_poll_head)(struct file *, __poll_t);
+ __poll_t (*poll_mask) (struct file *, __poll_t);
long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
int (*mmap) (struct file *, struct vm_area_struct *);
diff --git a/include/linux/poll.h b/include/linux/poll.h
index 7e0fdcf905d2..42e8e8665fb0 100644
--- a/include/linux/poll.h
+++ b/include/linux/poll.h
@@ -74,18 +74,37 @@ static inline void init_poll_funcptr(poll_table *pt, poll_queue_proc qproc)
pt->_key = ~(__poll_t)0; /* all events enabled */
}
+/*
+ * ->get_poll_head can return a __poll_t in the PTR_ERR, use these macros
+ * to return the value and recover it. It takes care of the negation as
+ * well as off the annotations.
+ */
+#define POLL_TO_PTR(mask) (ERR_PTR(-(__force int)(mask)))
+#define PTR_TO_POLL(ptr) ((__force __poll_t)-PTR_ERR((ptr)))
+
static inline bool file_can_poll(struct file *file)
{
- return file->f_op->poll;
+ return file->f_op->poll ||
+ (file->f_op->get_poll_head && file->f_op->poll_mask);
}
-static inline __poll_t vfs_poll(struct file *file, struct poll_table_struct *pt)
+static inline struct wait_queue_head *vfs_get_poll_head(struct file *file,
+ __poll_t events)
{
- if (unlikely(!file->f_op->poll))
+ if (unlikely(!file->f_op->get_poll_head || !file->f_op->poll_mask))
+ return NULL;
+ return file->f_op->get_poll_head(file, events);
+}
+
+static inline __poll_t vfs_poll_mask(struct file *file, __poll_t events)
+{
+ if (unlikely(!file->f_op->poll_mask))
return DEFAULT_POLLMASK;
- return file->f_op->poll(file, pt);
+ return file->f_op->poll_mask(file, events) & events;
}
+__poll_t vfs_poll(struct file *file, struct poll_table_struct *pt);
+
struct poll_table_entry {
struct file *filp;
__poll_t key;
--
2.14.2
--
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 related
* [PATCH 06/30] aio: simplify cancellation
From: Christoph Hellwig @ 2018-03-29 20:33 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
With the current aio code there is no need for the magic KIOCB_CANCELLED
value, as a cancelation just kicks the driver to queue the completion
ASAP, with all actual completion handling done in another thread. Given
that both the completion path and cancelation take the context lock there
is no need for magic cmpxchg loops either. If we remove iocbs from the
active list before calling ->ki_cancel we can also rely on the invariant
thay anything found on the list has a ->ki_cancel callback and can be
cancelled, further simplifing the code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/aio.c | 49 ++++++-------------------------------------------
1 file changed, 6 insertions(+), 43 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index 0df07d399a05..c724f1429176 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -162,19 +162,6 @@ struct fsync_iocb {
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
- * successful cancellation - io_cancel() - does deliver the completion to
- * userspace).
- *
- * And since most things don't implement kiocb cancellation and we'd really like
- * kiocb completion to be lockless when possible, we use ki_cancel to
- * synchronize cancellation and completion - we only set it to KIOCB_CANCELLED
- * with xchg() or cmpxchg(), see batch_complete_aio() and kiocb_cancel().
- */
-#define KIOCB_CANCELLED ((void *) (~0ULL))
-
struct aio_kiocb {
union {
struct kiocb rw;
@@ -572,27 +559,6 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
}
EXPORT_SYMBOL(kiocb_set_cancel_fn);
-static int kiocb_cancel(struct aio_kiocb *kiocb)
-{
- kiocb_cancel_fn *old, *cancel;
-
- /*
- * Don't want to set kiocb->ki_cancel = KIOCB_CANCELLED unless it
- * actually has a cancel function, hence the cmpxchg()
- */
-
- cancel = READ_ONCE(kiocb->ki_cancel);
- do {
- if (!cancel || cancel == KIOCB_CANCELLED)
- return -EINVAL;
-
- old = cancel;
- cancel = cmpxchg(&kiocb->ki_cancel, old, KIOCB_CANCELLED);
- } while (cancel != old);
-
- return cancel(&kiocb->rw);
-}
-
static void free_ioctx(struct work_struct *work)
{
struct kioctx *ctx = container_of(work, struct kioctx, free_work);
@@ -633,9 +599,8 @@ static void free_ioctx_users(struct percpu_ref *ref)
while (!list_empty(&ctx->active_reqs)) {
req = list_first_entry(&ctx->active_reqs,
struct aio_kiocb, ki_list);
-
list_del_init(&req->ki_list);
- kiocb_cancel(req);
+ req->ki_cancel(&req->rw);
}
spin_unlock_irq(&ctx->ctx_lock);
@@ -1837,8 +1802,8 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb,
{
struct kioctx *ctx;
struct aio_kiocb *kiocb;
+ int ret = -EINVAL;
u32 key;
- int ret;
ret = get_user(key, &iocb->aio_key);
if (unlikely(ret))
@@ -1849,13 +1814,11 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb,
return -EINVAL;
spin_lock_irq(&ctx->ctx_lock);
-
kiocb = lookup_kiocb(ctx, iocb, key);
- if (kiocb)
- ret = kiocb_cancel(kiocb);
- else
- ret = -EINVAL;
-
+ if (kiocb) {
+ list_del_init(&kiocb->ki_list);
+ ret = kiocb->ki_cancel(&kiocb->rw);
+ }
spin_unlock_irq(&ctx->ctx_lock);
if (!ret) {
--
2.14.2
--
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 related
* [PATCH 07/30] aio: add delayed cancel support
From: Christoph Hellwig @ 2018-03-29 20:33 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180329203328.3248-1-hch@lst.de>
The upcoming aio poll support would like to be able to complete the
iocb inline from the cancellation context, but that would cause a
double lock of ctx_lock with the current locking scheme. Move the
cancelation outside the context lock to avoid this reversal, which
suits the existing usb gadgets users just fine as well (in fact
both unconditionally disable irqs and thus seem broken without
this change).
To make this safe aio_complete needs to check if this call should
complete the iocb. If it didn't the callers must not release any
other resources.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/aio.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 44 insertions(+), 16 deletions(-)
diff --git a/fs/aio.c b/fs/aio.c
index c724f1429176..2406644e1ecc 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -177,6 +177,9 @@ struct aio_kiocb {
struct list_head ki_list; /* the aio core uses this
* for cancellation */
+ unsigned int flags; /* protected by ctx->ctx_lock */
+#define AIO_IOCB_CANCELLED (1 << 0)
+
/*
* If the aio_resfd field of the userspace iocb is not zero,
* this is the underlying eventfd context to deliver events to.
@@ -543,9 +546,9 @@ static int aio_setup_ring(struct kioctx *ctx, unsigned int nr_events)
#define AIO_EVENTS_FIRST_PAGE ((PAGE_SIZE - sizeof(struct aio_ring)) / sizeof(struct io_event))
#define AIO_EVENTS_OFFSET (AIO_EVENTS_PER_PAGE - AIO_EVENTS_FIRST_PAGE)
-void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
+static void __kiocb_set_cancel_fn(struct aio_kiocb *req,
+ kiocb_cancel_fn *cancel)
{
- struct aio_kiocb *req = container_of(iocb, struct aio_kiocb, rw);
struct kioctx *ctx = req->ki_ctx;
unsigned long flags;
@@ -557,6 +560,12 @@ void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
req->ki_cancel = cancel;
spin_unlock_irqrestore(&ctx->ctx_lock, flags);
}
+
+void kiocb_set_cancel_fn(struct kiocb *iocb, kiocb_cancel_fn *cancel)
+{
+ return __kiocb_set_cancel_fn(container_of(iocb, struct aio_kiocb, rw),
+ cancel);
+}
EXPORT_SYMBOL(kiocb_set_cancel_fn);
static void free_ioctx(struct work_struct *work)
@@ -593,18 +602,23 @@ static void free_ioctx_users(struct percpu_ref *ref)
{
struct kioctx *ctx = container_of(ref, struct kioctx, users);
struct aio_kiocb *req;
+ LIST_HEAD(list);
spin_lock_irq(&ctx->ctx_lock);
-
while (!list_empty(&ctx->active_reqs)) {
req = list_first_entry(&ctx->active_reqs,
struct aio_kiocb, ki_list);
+ req->flags |= AIO_IOCB_CANCELLED;
+ list_move_tail(&req->ki_list, &list);
+ }
+ spin_unlock_irq(&ctx->ctx_lock);
+
+ while (!list_empty(&list)) {
+ req = list_first_entry(&list, struct aio_kiocb, ki_list);
list_del_init(&req->ki_list);
req->ki_cancel(&req->rw);
}
- spin_unlock_irq(&ctx->ctx_lock);
-
percpu_ref_kill(&ctx->reqs);
percpu_ref_put(&ctx->reqs);
}
@@ -1040,22 +1054,30 @@ static struct kioctx *lookup_ioctx(unsigned long ctx_id)
return ret;
}
+#define AIO_COMPLETE_CANCEL (1 << 0)
+
/* aio_complete
* Called when the io request on the given iocb is complete.
*/
-static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
+static bool aio_complete(struct aio_kiocb *iocb, long res, long res2,
+ unsigned complete_flags)
{
struct kioctx *ctx = iocb->ki_ctx;
struct aio_ring *ring;
struct io_event *ev_page, *event;
unsigned tail, pos, head;
- unsigned long flags;
-
- if (!list_empty_careful(&iocb->ki_list)) {
- unsigned long flags;
+ unsigned long flags;
+ if (iocb->ki_cancel) {
spin_lock_irqsave(&ctx->ctx_lock, flags);
- list_del(&iocb->ki_list);
+ if (!(complete_flags & AIO_COMPLETE_CANCEL) &&
+ (iocb->flags & AIO_IOCB_CANCELLED)) {
+ spin_unlock_irqrestore(&ctx->ctx_lock, flags);
+ return false;
+ }
+
+ if (!list_empty(&iocb->ki_list))
+ list_del(&iocb->ki_list);
spin_unlock_irqrestore(&ctx->ctx_lock, flags);
}
@@ -1131,6 +1153,7 @@ static void aio_complete(struct aio_kiocb *iocb, long res, long res2)
wake_up(&ctx->wait);
percpu_ref_put(&ctx->reqs);
+ return true;
}
/* aio_read_events_ring
@@ -1379,6 +1402,7 @@ SYSCALL_DEFINE1(io_destroy, aio_context_t, ctx)
static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
{
struct aio_kiocb *iocb = container_of(kiocb, struct aio_kiocb, rw);
+ struct file *file = kiocb->ki_filp;
if (kiocb->ki_flags & IOCB_WRITE) {
struct inode *inode = file_inode(kiocb->ki_filp);
@@ -1392,8 +1416,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res, long res2)
file_end_write(kiocb->ki_filp);
}
- fput(kiocb->ki_filp);
- aio_complete(iocb, res, res2);
+ if (aio_complete(iocb, res, res2, 0))
+ fput(file);
}
static int aio_prep_rw(struct kiocb *req, struct iocb *iocb)
@@ -1536,11 +1560,13 @@ static ssize_t aio_write(struct kiocb *req, struct iocb *iocb, bool vectored,
static void aio_fsync_work(struct work_struct *work)
{
struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
+ struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
+ struct file *file = req->file;
int ret;
ret = vfs_fsync(req->file, req->datasync);
- fput(req->file);
- aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
+ if (aio_complete(iocb, ret, 0, 0))
+ fput(file);
}
static int aio_fsync(struct fsync_iocb *req, struct iocb *iocb, bool datasync)
@@ -1816,11 +1842,13 @@ SYSCALL_DEFINE3(io_cancel, aio_context_t, ctx_id, struct iocb __user *, iocb,
spin_lock_irq(&ctx->ctx_lock);
kiocb = lookup_kiocb(ctx, iocb, key);
if (kiocb) {
+ kiocb->flags |= AIO_IOCB_CANCELLED;
list_del_init(&kiocb->ki_list);
- ret = kiocb->ki_cancel(&kiocb->rw);
}
spin_unlock_irq(&ctx->ctx_lock);
+ if (kiocb)
+ ret = kiocb->ki_cancel(&kiocb->rw);
if (!ret) {
/*
* The result argument is no longer used - the io_event is
--
2.14.2
--
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 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