The first patch is a bit ugly, but it fixes a bug that could affect
lockdep. That bug is very minor and may not be observable at all,
but I don't really want to bet on it.
The other three are intended to fix a performance regression in the
entry rework that Frédéric objected to. They're much later than I'd
like to have sent them for 4.4, but they're kind-of sort-of
regression fixes, so maybe they're still okay. They would certainly
need careful review, though.
I don't have a great benchmark for them. The biggest impact is
likely to be to user page fault latency on CONFIG_CONTEXT_TRACKING=y
kernels (i.e. distro kernels) that don't use context tracking
(i.e. most users).
Andy Lutomirski (4):
x86/entry/64: Fix irqflag tracing wrt context tracking
context_tracking: Switch to new static_branch API
x86/asm: Add asm macros for static keys/jump labels
x86/entry/64: Bypass enter_from_user_mode on non-context-tracking
boots
arch/x86/entry/calling.h | 15 ++++++++++
arch/x86/entry/entry_64.S | 19 ++++++++-----
arch/x86/include/asm/jump_label.h | 52 ++++++++++++++++++++++++++++------
include/linux/context_tracking_state.h | 4 +--
kernel/context_tracking.c | 4 +--
5 files changed, 75 insertions(+), 19 deletions(-)
--
2.4.3
Paolo pointed out that enter_from_user_mode could be called while
irqflags were traced as though IRQs were on.
In principle, this could confuse lockdep. It doesn't cause any
problems that I've seen in any configuration, but if I build with
CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:
if (irqs_disabled()) {
spin_lock(&something);
spin_unlock(&something);
}
to the top of enter_from_user_mode, then lockdep will complain
without this fix. It seems that lockdep's irqflags sanity checks
are too weak to detect this bug without forcing the issue.
This patch adds one byte to normal kernels, and it's IMO a bit ugly.
I haven't spotted a better way to do this yet, though. The issue is
that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but
we're also supposed to do it before calling C code.
An alternative approach would be to call trace_hardirqs_off in
enter_from_user_mode. That would be less code and would not bloat
normal kernels at all, but it would be harder to see how the code
worked.
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/entry/entry_64.S | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 53616ca03244..f585df24ab3d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -509,6 +509,14 @@ END(irq_entries_start)
* tracking that we're in kernel mode.
*/
SWAPGS
+
+ /*
+ * IRQs are off. NB: this trace call is duplicated. That's
+ * okay -- it's idempotent and it's irrelevant for performance as
+ * it's a no-op unless CONFIG_DEBUG_LOCKDEP=y.
+ */
+ TRACE_IRQS_OFF
+
#ifdef CONFIG_CONTEXT_TRACKING
call enter_from_user_mode
#endif
@@ -1049,12 +1057,13 @@ ENTRY(error_entry)
SWAPGS
.Lerror_entry_from_usermode_after_swapgs:
+ TRACE_IRQS_OFF
#ifdef CONFIG_CONTEXT_TRACKING
call enter_from_user_mode
#endif
+ ret
.Lerror_entry_done:
-
TRACE_IRQS_OFF
ret
--
2.4.3
This is much less error-prone than the old code.
Signed-off-by: Andy Lutomirski <[email protected]>
---
include/linux/context_tracking_state.h | 4 ++--
kernel/context_tracking.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/context_tracking_state.h b/include/linux/context_tracking_state.h
index ee956c528fab..1d34fe68f48a 100644
--- a/include/linux/context_tracking_state.h
+++ b/include/linux/context_tracking_state.h
@@ -22,12 +22,12 @@ struct context_tracking {
};
#ifdef CONFIG_CONTEXT_TRACKING
-extern struct static_key context_tracking_enabled;
+extern struct static_key_false context_tracking_enabled;
DECLARE_PER_CPU(struct context_tracking, context_tracking);
static inline bool context_tracking_is_enabled(void)
{
- return static_key_false(&context_tracking_enabled);
+ return static_branch_unlikely(&context_tracking_enabled);
}
static inline bool context_tracking_cpu_is_enabled(void)
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index 0a495ab35bc7..d0fb09e40784 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -24,7 +24,7 @@
#define CREATE_TRACE_POINTS
#include <trace/events/context_tracking.h>
-struct static_key context_tracking_enabled = STATIC_KEY_INIT_FALSE;
+DEFINE_STATIC_KEY_FALSE(context_tracking_enabled);
EXPORT_SYMBOL_GPL(context_tracking_enabled);
DEFINE_PER_CPU(struct context_tracking, context_tracking);
@@ -191,7 +191,7 @@ void __init context_tracking_cpu_set(int cpu)
if (!per_cpu(context_tracking.active, cpu)) {
per_cpu(context_tracking.active, cpu) = true;
- static_key_slow_inc(&context_tracking_enabled);
+ static_branch_inc(&context_tracking_enabled);
}
if (initialized)
--
2.4.3
Unfortunately, these only work if HAVE_JUMP_LABEL. In principle, we
could do some serious surgery on the core jump label infrastructure
to keep the patch infrastructure available on x86 on all builds, but
that's probably not worth it.
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/include/asm/jump_label.h | 52 +++++++++++++++++++++++++++++++++------
1 file changed, 44 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index 5daeca3d0f9e..da275c1825f5 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -1,13 +1,6 @@
#ifndef _ASM_X86_JUMP_LABEL_H
#define _ASM_X86_JUMP_LABEL_H
-#ifndef __ASSEMBLY__
-
-#include <linux/stringify.h>
-#include <linux/types.h>
-#include <asm/nops.h>
-#include <asm/asm.h>
-
#define JUMP_LABEL_NOP_SIZE 5
#ifdef CONFIG_X86_64
@@ -16,6 +9,14 @@
# define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC
#endif
+#include <asm/asm.h>
+#include <asm/nops.h>
+
+#ifndef __ASSEMBLY__
+
+#include <linux/stringify.h>
+#include <linux/types.h>
+
static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
asm_volatile_goto("1:"
@@ -59,5 +60,40 @@ struct jump_entry {
jump_label_t key;
};
-#endif /* __ASSEMBLY__ */
+#else /* __ASSEMBLY__ */
+
+.macro STATIC_JUMP_IF_TRUE target, key, def
+.Lstatic_jump_\@:
+ .if \def
+ /* Equivalent to "jmp.d32 \target" */
+ .byte 0xe9
+ .long \target - .Lstatic_jump_after_\@
+.Lstatic_jump_after_\@:
+ .else
+ .byte STATIC_KEY_INIT_NOP
+ .endif
+ .pushsection __jump_table, "aw"
+ _ASM_ALIGN
+ _ASM_PTR .Lstatic_jump_\@, \target, \key
+ .popsection
+.endm
+
+.macro STATIC_JUMP_IF_FALSE target, key, def
+.Lstatic_jump_\@:
+ .if \def
+ .byte STATIC_KEY_INIT_NOP
+ .else
+ /* Equivalent to "jmp.d32 \target" */
+ .byte 0xe9
+ .long \target - .Lstatic_jump_after_\@
+.Lstatic_jump_after_\@:
+ .endif
+ .pushsection __jump_table, "aw"
+ _ASM_ALIGN
+ _ASM_PTR .Lstatic_jump_\@, \target, \key + 1
+ .popsection
+.endm
+
+#endif /* __ASSEMBLY__ */
+
#endif
--
2.4.3
On CONFIG_CONTEXT_TRACKING kernels that have context tracking
disabled at runtime (which includes most distro kernels), we still
have the overhead of a call to enter_from_user_mode in interrupt and
exception entries.
If jump labels are available, this uses the jump label
infrastructure to skip the call.
Signed-off-by: Andy Lutomirski <[email protected]>
---
arch/x86/entry/calling.h | 15 +++++++++++++++
arch/x86/entry/entry_64.S | 8 ++------
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 3c71dd947c7b..271e30c585bc 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -1,3 +1,5 @@
+#include <linux/jump_label.h>
+
/*
x86 function call convention, 64-bit:
@@ -232,3 +234,16 @@ For 32-bit we have the following conventions - kernel is built with
#endif /* CONFIG_X86_64 */
+/*
+ * This does 'call enter_from_user_mode' unless we can avoid it based on
+ * kernel config or using the static jump infrastructure.
+ */
+.macro CALL_ENTER_FROM_USER_MODE
+#ifdef CONFIG_CONTEXT_TRACKING
+#ifdef HAVE_JUMP_LABEL
+ STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
+#endif
+ call enter_from_user_mode
+.Lafter_call_\@:
+#endif
+.endm
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index f585df24ab3d..9b49b56efa29 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -517,9 +517,7 @@ END(irq_entries_start)
*/
TRACE_IRQS_OFF
-#ifdef CONFIG_CONTEXT_TRACKING
- call enter_from_user_mode
-#endif
+ CALL_ENTER_FROM_USER_MODE
1:
/*
@@ -1058,9 +1056,7 @@ ENTRY(error_entry)
.Lerror_entry_from_usermode_after_swapgs:
TRACE_IRQS_OFF
-#ifdef CONFIG_CONTEXT_TRACKING
- call enter_from_user_mode
-#endif
+ CALL_ENTER_FROM_USER_MODE
ret
.Lerror_entry_done:
--
2.4.3
On Fri, 6 Nov 2015, Andy Lutomirski wrote:
> Paolo pointed out that enter_from_user_mode could be called while
> irqflags were traced as though IRQs were on.
>
> In principle, this could confuse lockdep. It doesn't cause any
> problems that I've seen in any configuration, but if I build with
> CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:
>
> if (irqs_disabled()) {
> spin_lock(&something);
> spin_unlock(&something);
> }
>
> to the top of enter_from_user_mode, then lockdep will complain
> without this fix. It seems that lockdep's irqflags sanity checks
> are too weak to detect this bug without forcing the issue.
>
> This patch adds one byte to normal kernels, and it's IMO a bit ugly.
> I haven't spotted a better way to do this yet, though. The issue is
> that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but
> we're also supposed to do it before calling C code.
>
> An alternative approach would be to call trace_hardirqs_off in
> enter_from_user_mode. That would be less code and would not bloat
> normal kernels at all, but it would be harder to see how the code
> worked.
>
> Signed-off-by: Andy Lutomirski <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
On Fri, 6 Nov 2015, Andy Lutomirski wrote:
> This is much less error-prone than the old code.
>
> Signed-off-by: Andy Lutomirski <[email protected]>
Reviewed-by: Thomas Gleixner <[email protected]>
On Fri, Nov 06, 2015 at 03:12:43PM -0800, Andy Lutomirski wrote:
> Paolo pointed out that enter_from_user_mode could be called while
> irqflags were traced as though IRQs were on.
>
> In principle, this could confuse lockdep. It doesn't cause any
> problems that I've seen in any configuration, but if I build with
> CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:
>
> if (irqs_disabled()) {
> spin_lock(&something);
> spin_unlock(&something);
> }
>
> to the top of enter_from_user_mode, then lockdep will complain
> without this fix. It seems that lockdep's irqflags sanity checks
> are too weak to detect this bug without forcing the issue.
>
> This patch adds one byte to normal kernels, and it's IMO a bit ugly.
> I haven't spotted a better way to do this yet, though. The issue is
> that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but
> we're also supposed to do it before calling C code.
I would not mind to have that explanation in the code itself so that
people don't scratch heads why the duplicated TRACE_IRQS_OFF call.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
On Fri, 6 Nov 2015, Andy Lutomirski wrote:
> Unfortunately, these only work if HAVE_JUMP_LABEL. In principle, we
> could do some serious surgery on the core jump label infrastructure
> to keep the patch infrastructure available on x86 on all builds, but
> that's probably not worth it.
No, but we can be smarter about it. There is nothing which that asm
entry code needs from linux/jump_label.h and asm/jump_label.h execpt
STATIC_KEY_INIT_NOP. Something like the patch below should do the
trick.
Thanks,
tglx
Index: tip/arch/x86/entry/calling.h
===================================================================
--- tip.orig/arch/x86/entry/calling.h
+++ tip/arch/x86/entry/calling.h
@@ -232,3 +232,22 @@ For 32-bit we have the following convent
#endif /* CONFIG_X86_64 */
+/* ASM jump label support */
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
+#include <asm/jump_label.h>
+
+ .macro STATIC_CALL_IF_ENABLED fun, key
+ 1:
+ jmp.d32 2f
+ call fun
+ .pushsection __jump_table, "aw"
+ _ASM_ALIGN
+ _ASM_PTR 1b, 2f, \key
+ .popsection
+ 2:
+ .endm
+#else
+ .macro STATIC_CALL_IF_ENABLED fun, key
+ call fun
+ .endm
+#endif
Index: tip/arch/x86/entry/entry_64.S
===================================================================
--- tip.orig/arch/x86/entry/entry_64.S
+++ tip/arch/x86/entry/entry_64.S
@@ -510,7 +510,7 @@ END(irq_entries_start)
*/
SWAPGS
#ifdef CONFIG_CONTEXT_TRACKING
- call enter_from_user_mode
+ STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
#endif
1:
@@ -1050,7 +1050,7 @@ ENTRY(error_entry)
.Lerror_entry_from_usermode_after_swapgs:
#ifdef CONFIG_CONTEXT_TRACKING
- call enter_from_user_mode
+ STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
#endif
.Lerror_entry_done:
Index: tip/arch/x86/include/asm/jump_label.h
===================================================================
--- tip.orig/arch/x86/include/asm/jump_label.h
+++ tip/arch/x86/include/asm/jump_label.h
@@ -1,10 +1,6 @@
#ifndef _ASM_X86_JUMP_LABEL_H
#define _ASM_X86_JUMP_LABEL_H
-#ifndef __ASSEMBLY__
-
-#include <linux/stringify.h>
-#include <linux/types.h>
#include <asm/nops.h>
#include <asm/asm.h>
@@ -16,6 +12,11 @@
# define STATIC_KEY_INIT_NOP GENERIC_NOP5_ATOMIC
#endif
+#ifndef __ASSEMBLY__
+
+#include <linux/stringify.h>
+#include <linux/types.h>
+
static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
asm_volatile_goto("1:"
On Nov 7, 2015 3:21 AM, "Thomas Gleixner" <[email protected]> wrote:
>
> On Fri, 6 Nov 2015, Andy Lutomirski wrote:
>
> > Unfortunately, these only work if HAVE_JUMP_LABEL. In principle, we
> > could do some serious surgery on the core jump label infrastructure
> > to keep the patch infrastructure available on x86 on all builds, but
> > that's probably not worth it.
>
> No, but we can be smarter about it. There is nothing which that asm
> entry code needs from linux/jump_label.h and asm/jump_label.h execpt
> STATIC_KEY_INIT_NOP. Something like the patch below should do the
> trick.
I like some bits of this. See below.
>
> Thanks,
>
> tglx
>
> Index: tip/arch/x86/entry/calling.h
> ===================================================================
> --- tip.orig/arch/x86/entry/calling.h
> +++ tip/arch/x86/entry/calling.h
> @@ -232,3 +232,22 @@ For 32-bit we have the following convent
>
> #endif /* CONFIG_X86_64 */
>
> +/* ASM jump label support */
> +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
> +#include <asm/jump_label.h>
> +
> + .macro STATIC_CALL_IF_ENABLED fun, key
I think we still need the "def" parameter. But maybe the "static
call" is a good idea. (By giving 'def' a default value or omitting it
entirely, it's too easy not to think about it, and that's a huge
historical source of breakage and it's a good part of the reason for
the new improved C API. At least these days I *think* we'll diagnose
the problem on bootup instead of waiting for random breakage later.)
> + 1:
> + jmp.d32 2f
Old binutils will choke on this. But maybe no one has jump labels and
old binutils.
> + call fun
> + .pushsection __jump_table, "aw"
> + _ASM_ALIGN
> + _ASM_PTR 1b, 2f, \key
> + .popsection
> + 2:
> + .endm
> +#else
> + .macro STATIC_CALL_IF_ENABLED fun, key
> + call fun
> + .endm
This could result in errors down the road, since this macro is really
"static call if enabled or if the kernel is built without jump
labels". enter_from_user_mode is special because it's already a no-op
if context tracking is off, but I'm not sure that this oddity should
leak into more generic code.
> +#endif
> Index: tip/arch/x86/entry/entry_64.S
> ===================================================================
> --- tip.orig/arch/x86/entry/entry_64.S
> +++ tip/arch/x86/entry/entry_64.S
> @@ -510,7 +510,7 @@ END(irq_entries_start)
> */
> SWAPGS
> #ifdef CONFIG_CONTEXT_TRACKING
> - call enter_from_user_mode
> + STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
> #endif
This doesn't get rid of the ifdeffery, which I thought was a nice
feature of my patch.
--Andy
On Sat, 7 Nov 2015, Andy Lutomirski wrote:
> On Nov 7, 2015 3:21 AM, "Thomas Gleixner" <[email protected]> wrote:
> > +/* ASM jump label support */
> > +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
> > +#include <asm/jump_label.h>
> > +
> > + .macro STATIC_CALL_IF_ENABLED fun, key
>
> I think we still need the "def" parameter. But maybe the "static
> call" is a good idea. (By giving 'def' a default value or omitting it
> entirely, it's too easy not to think about it, and that's a huge
> historical source of breakage and it's a good part of the reason for
> the new improved C API. At least these days I *think* we'll diagnose
> the problem on bootup instead of waiting for random breakage later.)
Ok. That's easy to add :)
> > + 1:
> > + jmp.d32 2f
>
> Old binutils will choke on this. But maybe no one has jump labels and
> old binutils.
Good question.
> > + call fun
> > + .pushsection __jump_table, "aw"
> > + _ASM_ALIGN
> > + _ASM_PTR 1b, 2f, \key
> > + .popsection
> > + 2:
> > + .endm
> > +#else
> > + .macro STATIC_CALL_IF_ENABLED fun, key
> > + call fun
> > + .endm
>
> This could result in errors down the road, since this macro is really
> "static call if enabled or if the kernel is built without jump
> labels". enter_from_user_mode is special because it's already a no-op
> if context tracking is off, but I'm not sure that this oddity should
> leak into more generic code.
Hmm. Maybe we need a better name for this macro.
> > +#endif
> > Index: tip/arch/x86/entry/entry_64.S
> > ===================================================================
> > --- tip.orig/arch/x86/entry/entry_64.S
> > +++ tip/arch/x86/entry/entry_64.S
> > @@ -510,7 +510,7 @@ END(irq_entries_start)
> > */
> > SWAPGS
> > #ifdef CONFIG_CONTEXT_TRACKING
> > - call enter_from_user_mode
> > + STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
> > #endif
>
> This doesn't get rid of the ifdeffery, which I thought was a nice
> feature of my patch.
Right. I was just too lazy to add another macro :)
My main point was to avoid the extra stuff for !HAVE_JUMP_LABEL and
hide this in a header file.
Thanks,
tglx
On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
> On Sat, 7 Nov 2015, Andy Lutomirski wrote:
>> On Nov 7, 2015 3:21 AM, "Thomas Gleixner" <[email protected]> wrote:
>> > +/* ASM jump label support */
>> > +#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
>> > +#include <asm/jump_label.h>
>> > +
>> > + .macro STATIC_CALL_IF_ENABLED fun, key
>>
>> I think we still need the "def" parameter. But maybe the "static
>> call" is a good idea. (By giving 'def' a default value or omitting it
>> entirely, it's too easy not to think about it, and that's a huge
>> historical source of breakage and it's a good part of the reason for
>> the new improved C API. At least these days I *think* we'll diagnose
>> the problem on bootup instead of waiting for random breakage later.)
>
> Ok. That's easy to add :)
>
>> > + 1:
>> > + jmp.d32 2f
>>
>> Old binutils will choke on this. But maybe no one has jump labels and
>> old binutils.
>
> Good question.
>
>> > + call fun
>> > + .pushsection __jump_table, "aw"
>> > + _ASM_ALIGN
>> > + _ASM_PTR 1b, 2f, \key
>> > + .popsection
>> > + 2:
>> > + .endm
>> > +#else
>> > + .macro STATIC_CALL_IF_ENABLED fun, key
>> > + call fun
>> > + .endm
>>
>> This could result in errors down the road, since this macro is really
>> "static call if enabled or if the kernel is built without jump
>> labels". enter_from_user_mode is special because it's already a no-op
>> if context tracking is off, but I'm not sure that this oddity should
>> leak into more generic code.
>
> Hmm. Maybe we need a better name for this macro.
>
>> > +#endif
>> > Index: tip/arch/x86/entry/entry_64.S
>> > ===================================================================
>> > --- tip.orig/arch/x86/entry/entry_64.S
>> > +++ tip/arch/x86/entry/entry_64.S
>> > @@ -510,7 +510,7 @@ END(irq_entries_start)
>> > */
>> > SWAPGS
>> > #ifdef CONFIG_CONTEXT_TRACKING
>> > - call enter_from_user_mode
>> > + STATIC_CALL_IF_ENABLED enter_from_user_mode, context_tracking_enabled
>> > #endif
>>
>> This doesn't get rid of the ifdeffery, which I thought was a nice
>> feature of my patch.
>
> Right. I was just too lazy to add another macro :)
>
> My main point was to avoid the extra stuff for !HAVE_JUMP_LABEL and
> hide this in a header file.
>
True. But I hid it in a header file, too, but it was just a different
header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.
--Andy
On Sat, 7 Nov 2015, Andy Lutomirski wrote:
> On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
> True. But I hid it in a header file, too, but it was just a different
> header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.
It probably does not really make a difference :)
I just got triggered by you saying:
> Unfortunately, these only work if HAVE_JUMP_LABEL ....
Thanks,
tglx
On Sat, Nov 7, 2015 at 9:08 AM, Thomas Gleixner <[email protected]> wrote:
> On Sat, 7 Nov 2015, Andy Lutomirski wrote:
>> On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
>> True. But I hid it in a header file, too, but it was just a different
>> header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.
>
> It probably does not really make a difference :)
>
> I just got triggered by you saying:
>
>> Unfortunately, these only work if HAVE_JUMP_LABEL ....
Yeah, I don't really like that either. I think that the real fix
would be to compile in the runtime jump label bits unconditionally.
Admittedly it would bloat the kernel image a little bit for
!CONFIG_JUMP_LABEL or if build with an older gcc, and that's even less
appropriate for 4.4 at this point. Peter?
--Andy
On Sat, Nov 7, 2015 at 9:08 AM, Thomas Gleixner <[email protected]> wrote:
> On Sat, 7 Nov 2015, Andy Lutomirski wrote:
>> On Sat, Nov 7, 2015 at 8:58 AM, Thomas Gleixner <[email protected]> wrote:
>> True. But I hid it in a header file, too, but it was just a different
>> header file -- I had it all hidden away as CALL_ENTER_FROM_USER_MODE.
>
> It probably does not really make a difference :)
>
> I just got triggered by you saying:
>
>> Unfortunately, these only work if HAVE_JUMP_LABEL ....
>
Looking again, my patch is crappy is one obvious respect: on
non-HAVE_JUMP_LABEL kernels, I'm still generating a macro that will
compile but do the wrong thing. I'll add better comments and the
correct ifdef in v2. That will make it much harder to screw up
without causing a compiler error.
--Andy
On Sat, Nov 7, 2015 at 3:18 AM, Borislav Petkov <[email protected]> wrote:
> On Fri, Nov 06, 2015 at 03:12:43PM -0800, Andy Lutomirski wrote:
>> Paolo pointed out that enter_from_user_mode could be called while
>> irqflags were traced as though IRQs were on.
>>
>> In principle, this could confuse lockdep. It doesn't cause any
>> problems that I've seen in any configuration, but if I build with
>> CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:
>>
>> if (irqs_disabled()) {
>> spin_lock(&something);
>> spin_unlock(&something);
>> }
>>
>> to the top of enter_from_user_mode, then lockdep will complain
>> without this fix. It seems that lockdep's irqflags sanity checks
>> are too weak to detect this bug without forcing the issue.
>>
>> This patch adds one byte to normal kernels, and it's IMO a bit ugly.
>> I haven't spotted a better way to do this yet, though. The issue is
>> that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but
>> we're also supposed to do it before calling C code.
>
> I would not mind to have that explanation in the code itself so that
> people don't scratch heads why the duplicated TRACE_IRQS_OFF call.
>
Done for v2.
--Andy
* Andy Lutomirski <[email protected]> wrote:
> On CONFIG_CONTEXT_TRACKING kernels that have context tracking
> disabled at runtime (which includes most distro kernels), we still
> have the overhead of a call to enter_from_user_mode in interrupt and
> exception entries.
>
> If jump labels are available, this uses the jump label
> infrastructure to skip the call.
>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
> arch/x86/entry/calling.h | 15 +++++++++++++++
> arch/x86/entry/entry_64.S | 8 ++------
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 3c71dd947c7b..271e30c585bc 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -1,3 +1,5 @@
> +#include <linux/jump_label.h>
> +
> /*
>
> x86 function call convention, 64-bit:
> @@ -232,3 +234,16 @@ For 32-bit we have the following conventions - kernel is built with
>
> #endif /* CONFIG_X86_64 */
>
> +/*
> + * This does 'call enter_from_user_mode' unless we can avoid it based on
> + * kernel config or using the static jump infrastructure.
> + */
> +.macro CALL_ENTER_FROM_USER_MODE
> +#ifdef CONFIG_CONTEXT_TRACKING
> +#ifdef HAVE_JUMP_LABEL
> + STATIC_JUMP_IF_FALSE .Lafter_call_\@, context_tracking_enabled, def=0
> +#endif
> + call enter_from_user_mode
> +.Lafter_call_\@:
> +#endif
> +.endm
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index f585df24ab3d..9b49b56efa29 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -517,9 +517,7 @@ END(irq_entries_start)
> */
> TRACE_IRQS_OFF
>
> -#ifdef CONFIG_CONTEXT_TRACKING
> - call enter_from_user_mode
> -#endif
> + CALL_ENTER_FROM_USER_MODE
>
> 1:
> /*
> @@ -1058,9 +1056,7 @@ ENTRY(error_entry)
>
> .Lerror_entry_from_usermode_after_swapgs:
> TRACE_IRQS_OFF
> -#ifdef CONFIG_CONTEXT_TRACKING
> - call enter_from_user_mode
> -#endif
> + CALL_ENTER_FROM_USER_MODE
> ret
Yeah, so I only have a really minor syntactic nit abou tthis: it's OK to
capitalize things when doing macros, but here I don't think capitalizing the
called function name is good - I think the following would be more obvious:
CALL_enter_from_user_mode
this makes it really, really obvious (to me!) that this macro is a shortcut for:
call enter_from_user_mode
with some glue around it, and I could grep for 'enter_from_user_mode' immediately
- while grepping for ENTER_FROM_USER_MODE would miss the called function.
Thanks,
Ingo
On Sat, Nov 07, 2015 at 10:16:37AM -0800, Andy Lutomirski wrote:
> >> Unfortunately, these only work if HAVE_JUMP_LABEL ....
>
> Yeah, I don't really like that either. I think that the real fix
> would be to compile in the runtime jump label bits unconditionally.
> Admittedly it would bloat the kernel image a little bit for
> !CONFIG_JUMP_LABEL or if build with an older gcc, and that's even less
> appropriate for 4.4 at this point. Peter?
That is a _lot_ of code for just one user. I'll bet the tiny people
won't be happy about that.