From: Chris Metcalf <hidden> Date: 2016-01-04 19:37:15
This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability. The existing
arm64 do_notify_resume() is called in a loop from assembly on
the slow path; this change moves the loop into C code as well.
For the x86 version see commit c5c46f59e4e7 ("x86/entry: Add new,
comprehensible entry and exit handlers written in C").
Signed-off-by: Chris Metcalf <redacted>
---
arch/arm64/kernel/entry.S | 6 +++---
arch/arm64/kernel/signal.c | 32 ++++++++++++++++++++++----------
2 files changed, 25 insertions(+), 13 deletions(-)
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-04 20:33:39
Hi,
On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability.
I have also been looking into converting the userspace return path from
assembly to C [1], for the latter two reasons. Based on that, I have a
couple of comments.
quoted hunk
The existing arm64 do_notify_resume() is called in a loop from
assembly on the slow path; this change moves the loop into C code as
well. For the x86 version see commit c5c46f59e4e7 ("x86/entry: Add
new, comprehensible entry and exit handlers written in C").
Signed-off-by: Chris Metcalf <redacted>
---
arch/arm64/kernel/entry.S | 6 +++---
arch/arm64/kernel/signal.c | 32 ++++++++++++++++++++++----------
2 files changed, 25 insertions(+), 13 deletions(-)
It seems unfortunate to leave behind portions of the entry.S
_TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
and the majority of work_pending and ret_to_user).
I think it would be nicer if we could handle all of that in one place
(or at least all in C).
Previously, had we called schedule(), we'd reload the thread info flags
and start that state machine again, whereas now we'll handle all the
cached flags before reloading.
Are we sure nothing is relying on the prior behaviour?
quoted hunk
++ if (thread_flags & _TIF_SIGPENDING)+ do_signal(regs);++ if (thread_flags & _TIF_NOTIFY_RESUME) {+ clear_thread_flag(TIF_NOTIFY_RESUME);+ tracehook_notify_resume(regs);+ }++ if (thread_flags & _TIF_FOREIGN_FPSTATE)+ fpsimd_restore_current_state();++ local_irq_disable();- if (thread_flags & _TIF_FOREIGN_FPSTATE)- fpsimd_restore_current_state();+ thread_flags = READ_ONCE(current_thread_info()->flags) &+ _TIF_WORK_MASK;+ } while (thread_flags); }
From: Chris Metcalf <hidden> Date: 2016-01-04 21:01:28
On 01/04/2016 03:33 PM, Mark Rutland wrote:
Hi,
On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
quoted
This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability.
I have also been looking into converting the userspace return path from
assembly to C [1], for the latter two reasons. Based on that, I have a
couple of comments.
Thanks!
It seems unfortunate to leave behind portions of the entry.S
_TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
and the majority of work_pending and ret_to_user).
I think it would be nicer if we could handle all of that in one place
(or at least all in C).
Yes, in principle I agree with this, and I think your deasm tree looks
like an excellent idea.
For this patch series I wanted to focus more on what was necessary
for the various platforms to implement task isolation, and less on
additional cleanups of the platforms in question. I think my changes
don't make the TIF state machine any less clear, nor do they make
it harder for an eventual further migration to C code along the lines
of what you've done, so it seems plausible to me to commit them
upstream independently of your work.
Previously, had we called schedule(), we'd reload the thread info flags
and start that state machine again, whereas now we'll handle all the
cached flags before reloading.
Are we sure nothing is relying on the prior behaviour?
Good eye, and I probably should have called that out in the commit
message. My best guess is that there should be nothing that depends
on the old semantics. Other platforms (certainly x86 and tile, anyway)
already have the semantics that you run out the old state machine on
return from schedule(), so regardless, it's probably appropriate for
arm to follow that same convention.
quoted
++ if (thread_flags & _TIF_SIGPENDING)+ do_signal(regs);++ if (thread_flags & _TIF_NOTIFY_RESUME) {+ clear_thread_flag(TIF_NOTIFY_RESUME);+ tracehook_notify_resume(regs);+ }++ if (thread_flags & _TIF_FOREIGN_FPSTATE)+ fpsimd_restore_current_state();++ local_irq_disable();- if (thread_flags & _TIF_FOREIGN_FPSTATE)- fpsimd_restore_current_state();+ thread_flags = READ_ONCE(current_thread_info()->flags) &+ _TIF_WORK_MASK;+ } while (thread_flags); }
From: Andy Lutomirski <luto@amacapital.net> Date: 2016-01-04 22:32:07
On Mon, Jan 4, 2016 at 12:33 PM, Mark Rutland [off-list ref] wrote:
Hi,
On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
quoted
This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability.
I have also been looking into converting the userspace return path from
assembly to C [1], for the latter two reasons. Based on that, I have a
couple of comments.
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-05 17:21:35
On Mon, Jan 04, 2016 at 04:01:05PM -0500, Chris Metcalf wrote:
On 01/04/2016 03:33 PM, Mark Rutland wrote:
quoted
Hi,
On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
quoted
This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability.
I have also been looking into converting the userspace return path from
assembly to C [1], for the latter two reasons. Based on that, I have a
couple of comments.
Thanks!
quoted
It seems unfortunate to leave behind portions of the entry.S
_TIF_WORK_MASK state machine (i.e. a small portion of ret_fast_syscall,
and the majority of work_pending and ret_to_user).
I think it would be nicer if we could handle all of that in one place
(or at least all in C).
Yes, in principle I agree with this, and I think your deasm tree looks
like an excellent idea.
For this patch series I wanted to focus more on what was necessary
for the various platforms to implement task isolation, and less on
additional cleanups of the platforms in question. I think my changes
don't make the TIF state machine any less clear, nor do they make
it harder for an eventual further migration to C code along the lines
of what you've done, so it seems plausible to me to commit them
upstream independently of your work.
I appreciate that you don't want to rewrite all the code.
However, I think it's easier to factor out a small amount of additional
code now and evlove that as a whole than it will be to evolve part of it
and try to put it back together later.
I have a patch which I will reply with momentarily.
Thanks,
Mark.
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-05 17:34:00
In work_pending we may skip work if the stacked SPSR value represents
anything other than an EL0 context. We then immediately invoke the
kernel_exit 0 macro as part of ret_to_user, assuming a return to EL0.
This is somewhat confusing.
We use work_pending as part of the ret_to_user/ret_fast_syscall state
machine. We only use ret_fast_syscall in the return from an SVC issued
from EL0. We use ret_to_user for return from EL0 exception handlers and
also for return from ret_from_fork in the case the task was not a kernel
thread (i.e. it is a user task).
Thus in all cases the stacked SPSR value must represent an EL0 context,
and the check is redundant. This patch removes it, along with the now
unused no_work_pending label.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <redacted>
Cc: Will Deacon <redacted>
---
arch/arm64/kernel/entry.S | 4 ----
1 file changed, 4 deletions(-)
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-05 17:34:05
Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.
This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.
This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <redacted>
Cc: Will Deacon <redacted>
---
arch/arm64/kernel/entry.S | 24 +++---------------------
arch/arm64/kernel/signal.c | 36 ++++++++++++++++++++++++++----------
2 files changed, 29 insertions(+), 31 deletions(-)
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-05 18:01:33
On Mon, Jan 04, 2016 at 02:31:42PM -0800, Andy Lutomirski wrote:
On Mon, Jan 4, 2016 at 12:33 PM, Mark Rutland [off-list ref] wrote:
quoted
Hi,
On Mon, Jan 04, 2016 at 02:34:46PM -0500, Chris Metcalf wrote:
quoted
This change is a prerequisite change for TASK_ISOLATION but also
stands on its own for readability and maintainability.
I have also been looking into converting the userspace return path from
assembly to C [1], for the latter two reasons. Based on that, I have a
couple of comments.
It was in fact your x86 effort that inspired me to look at this!
Thanks for the pointer, I'm almost certainly going to steal an idea or
two.
Currently it looks like arm64's conversion will be less painful than
that for x86 as the entry assembly is smaller and relatively uniform.
It looks like all but the register save/restore is possible in C.
That said, I have yet to stress/validate everything with tracing, irq
debugging, and so on, so my confidence may be misplaced.
Thanks,
Mark.
From: Chris Metcalf <hidden> Date: 2016-01-05 18:53:19
On 01/05/2016 12:33 PM, Mark Rutland wrote:
Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.
This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.
This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.
Signed-off-by: Mark Rutland<mark.rutland@arm.com>
Cc: Catalin Marinas<catalin.marinas@arm.com>
Cc: Chris Metcalf<redacted>
Cc: Will Deacon<redacted>
---
arch/arm64/kernel/entry.S | 24 +++---------------------
arch/arm64/kernel/signal.c | 36 ++++++++++++++++++++++++++----------
2 files changed, 29 insertions(+), 31 deletions(-)
This looks good, and also makes the task isolation change drop in
very cleanly (relatively speaking). Since do_notify_resume() is
called unconditionally now, we don't have to worry about fussing
with the bit numbering for the TIF_xxx flags in asm/threadinfo.h, so
that whole part of the patch can be dropped, and the actual
change to do_notify_resume() becomes:
For the moment I just added your two commits into my task-isolation
tree and pushed it up, but if your changes make it into 4.5 and the
task-isolation series doesn't, I will remove them and rebase on 4.5-rc1
once that's released. I've similarly staged the arch/tile enablement
changes to go into 4.5 so I can drop them from the task-isolation tree
as well at that point.
--
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com
On Tue, Jan 05, 2016 at 05:33:34PM +0000, Mark Rutland wrote:
In work_pending we may skip work if the stacked SPSR value represents
anything other than an EL0 context. We then immediately invoke the
kernel_exit 0 macro as part of ret_to_user, assuming a return to EL0.
This is somewhat confusing.
We use work_pending as part of the ret_to_user/ret_fast_syscall state
machine. We only use ret_fast_syscall in the return from an SVC issued
from EL0. We use ret_to_user for return from EL0 exception handlers and
also for return from ret_from_fork in the case the task was not a kernel
thread (i.e. it is a user task).
Thus in all cases the stacked SPSR value must represent an EL0 context,
and the check is redundant. This patch removes it, along with the now
unused no_work_pending label.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <redacted>
Cc: Will Deacon <redacted>
On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.
This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.
This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <redacted>
Cc: Will Deacon <redacted>
This is definitely cleaner. The only downside is slightly more expensive
ret_fast_syscall. I guess it's not noticeable (though we could do some
quick benchmark like getpid in a loop). Anyway, I'm fine with the patch:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-06 12:48:16
On Wed, Jan 06, 2016 at 12:30:11PM +0000, Catalin Marinas wrote:
On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
quoted
Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.
This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.
This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Metcalf <redacted>
Cc: Will Deacon <redacted>
This is definitely cleaner. The only downside is slightly more expensive
ret_fast_syscall. I guess it's not noticeable (though we could do some
quick benchmark like getpid in a loop). Anyway, I'm fine with the patch:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Cheers!
While any additional overhead hasn't been noticeable, I'll try to get
some numbers out as part of the larger deasm testing/benchmarking.
Thanks,
Mark.
From: Mark Rutland <mark.rutland@arm.com> Date: 2016-01-06 13:43:36
On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.
This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.
This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.
I realised there is a problem with this for kernel built with
TRACE_IRQFLAGS, as local_irq_{enable,disable}() will verify that the IRQ
state is as expected.
In ret_fast_syscall we disable irqs behind the back of the tracer, so
when we get into do_notify_resume we'll get a splat.
In the non-syscall cases we do not disable interrupts first, so we can't
balance things in do_notify_resume.
We can either add a trace_hardirqs_off call to ret_fast_syscall, or we
can use raw_local_irq_{disable,enable}. The latter would match the
current behaviour (and is a nicer diff). Once the syscall path is moved
to C it would be possible to use the non-raw variants all-over.
Catalin, are you happy with using the raw accessors in do_notify_resume,
or would you prefer using trace_hardirqs_off?
Thanks,
Mark.
On Wed, Jan 06, 2016 at 01:43:14PM +0000, Mark Rutland wrote:
On Tue, Jan 05, 2016 at 05:33:35PM +0000, Mark Rutland wrote:
quoted
Currently ret_fast_syscall, work_pending, and ret_to_user form an ad-hoc
state machine that can be difficult to reason about due to duplicated
code and a large number of branch targets.
This patch factors the common logic out into the existing
do_notify_resume function, converting the code to C in the process,
making the code more legible.
This patch tries to mirror the existing behaviour as closely as possible
while using the usual C control flow primitives. There should be no
functional change as a result of this patch.
I realised there is a problem with this for kernel built with
TRACE_IRQFLAGS, as local_irq_{enable,disable}() will verify that the IRQ
state is as expected.
In ret_fast_syscall we disable irqs behind the back of the tracer, so
when we get into do_notify_resume we'll get a splat.
In the non-syscall cases we do not disable interrupts first, so we can't
balance things in do_notify_resume.
We can either add a trace_hardirqs_off call to ret_fast_syscall, or we
can use raw_local_irq_{disable,enable}. The latter would match the
current behaviour (and is a nicer diff). Once the syscall path is moved
to C it would be possible to use the non-raw variants all-over.
Catalin, are you happy with using the raw accessors in do_notify_resume,
or would you prefer using trace_hardirqs_off?
I would prefer the explicit trace_hardirqs_off annotation, even though
it is a few more lines.
--
Catalin