[PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Ian Campbell]

The following series removes the use of a global kernel_eflags variable
from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
64 bit version of that code path.

kernel_eflags could be made a __read_mostly but actually there is no
reason to prefer the value at cpu_init() time to a compile time constant
value for the initial eflags after a fork.

Ian.

[PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.

[Ian Campbell]

On 64 bit x86 we save the current eflags in cpu_init for use in ret_from_fork.
Strictly speaking reserved bits in EFLAGS should be read as written but in
practise it is unlikely that EFLAGS could ever be extended in this way and the
kernel alread clears any undefined flags early on

The equivalent 32 bit code simply hard codes 0x0202 as the new EFLAGS (fixed to
use symbolic constants in a subsequent patch).

This change makes 64 bit use the same mechanism to setup the initial EFLAGS on
fork. Note that 64 bit resets EFLAGS before calling schedule_tail() as opposed
to 32 bit which calls schedule_tail() first. Therefore the correct value for
EFLAGS has opposite IF bit. This will be fixed in a subsequent patch.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: x86@kernel.org
---
 arch/x86/include/asm/processor.h |    1 -
 arch/x86/kernel/cpu/common.c     |    4 ----
 arch/x86/kernel/entry_64.S       |    2 +-
 3 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2193715..698af88 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -398,7 +398,6 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
 
 DECLARE_PER_CPU(char *, irq_stack_ptr);
 DECLARE_PER_CPU(unsigned int, irq_count);
-extern unsigned long kernel_eflags;
 extern asmlinkage void ignore_sysret(void);
 #else	/* X86_64 */
 #ifdef CONFIG_CC_STACKPROTECTOR
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 22a073d..178c3b4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1077,8 +1077,6 @@ void syscall_init(void)
 	       X86_EFLAGS_TF|X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_IOPL);
 }
 
-unsigned long kernel_eflags;
-
 /*
  * Copies of the original ist values from the tss are only accessed during
  * debugging, no special alignment required.
@@ -1231,8 +1229,6 @@ void __cpuinit cpu_init(void)
 	fpu_init();
 	xsave_init();
 
-	raw_local_save_flags(kernel_eflags);
-
 	if (is_uv_system())
 		uv_cpu_init();
 }
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index e13329d..b50d142 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -396,7 +396,7 @@ ENTRY(ret_from_fork)
 
 	LOCK ; btr $TIF_FORK,TI_flags(%r8)
 
-	pushq_cfi kernel_eflags(%rip)
+	pushq_cfi $0x0002
 	popfq_cfi				# reset kernel eflags
 
 	call schedule_tail			# rdi: 'prev' task parameter
-- 
1.7.2.5

[PATCH 2/3] x86: make 64 bit ret_from_fork a little more similar to 32 bit

[Ian Campbell]

The 64 bit version resets EFLAGS before calling schedule_tail() and
therefore leaves EFLAGS.IF clear. 32 bit resets EFLAGS after calling
schedule_tail() and therefore leaves EFLAGS.IF set. I don't think
there is any practical difference between the two approaches since
interrupts are actually reenabled within schedule_tail
(schedule_tail->finish_task_switch->finish_lock_switch->raw_spin_unlock_irq->...->local_irq_enable)
so arbitrarily pick the 32 bit version and make 64 bit look like that.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Pekka Enberg <penberg@kernel.org>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/entry_64.S |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index b50d142..9c28bd5 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -394,13 +394,14 @@ END(save_paranoid)
 ENTRY(ret_from_fork)
 	DEFAULT_FRAME
 
-	LOCK ; btr $TIF_FORK,TI_flags(%r8)
 
-	pushq_cfi $0x0002
-	popfq_cfi				# reset kernel eflags
+	LOCK ; btr $TIF_FORK,TI_flags(%r8)
 
 	call schedule_tail			# rdi: 'prev' task parameter
 
+	pushq_cfi $0x0202
+	popfq_cfi				# reset kernel eflags
+
 	GET_THREAD_INFO(%rcx)
 
 	RESTORE_REST
-- 
1.7.2.5

[PATCH 3/3] x86: ret_from_fork: use symbolic contants for bits in EFLAGS

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Pekka Enberg <penberg@kernel.org>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: x86@kernel.org
---
 arch/x86/kernel/entry_32.S |    2 +-
 arch/x86/kernel/entry_64.S |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 5c1a9197..f032530 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -303,7 +303,7 @@ ENTRY(ret_from_fork)
 	call schedule_tail
 	GET_THREAD_INFO(%ebp)
 	popl_cfi %eax
-	pushl_cfi $0x0202		# Reset kernel eflags
+	pushl_cfi $(X86_EFLAGS_IF|0x2)	# Reset kernel eflags
 	popfl_cfi
 	jmp syscall_exit
 	CFI_ENDPROC
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 9c28bd5..4c76abf 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -399,7 +399,7 @@ ENTRY(ret_from_fork)
 
 	call schedule_tail			# rdi: 'prev' task parameter
 
-	pushq_cfi $0x0202
+	pushq_cfi $(X86_EFLAGS_IF|0x2)
 	popfq_cfi				# reset kernel eflags
 
 	GET_THREAD_INFO(%rcx)
-- 
1.7.2.5

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Cyrill Gorcunov]

On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
> The following series removes the use of a global kernel_eflags variable
> from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
> 64 bit version of that code path.
> 
> kernel_eflags could be made a __read_mostly but actually there is no
> reason to prefer the value at cpu_init() time to a compile time constant
> value for the initial eflags after a fork.
> 
> Ian.
> 

Thanks, Ian! I think noone against this simplification, Peter, Andi?

	Cyrill

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Cyrill Gorcunov]

On Mon, Jul 25, 2011 at 02:19:02PM +0400, Cyrill Gorcunov wrote:
> On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
> > The following series removes the use of a global kernel_eflags variable
> > from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
> > 64 bit version of that code path.
> > 
> > kernel_eflags could be made a __read_mostly but actually there is no
> > reason to prefer the value at cpu_init() time to a compile time constant
> > value for the initial eflags after a fork.
> > 
> > Ian.
> > 
> 
> Thanks, Ian! I think noone against this simplification, Peter, Andi?
> 
> 	Cyrill

Ian, I've missed in first place that you've opened IRQs window _before_
schedule_tail() call, ie it's not 1:1 code mapping as it was before.

Note kernel_eflags has IF clear and what we have: the ret_from_fork on
x86-64 happens _only_ inside context_switch call, ie

schedule (sched.c)
        ...
        raw_spin_lock_irq
        ...
        context_switch
                switch_to
                        "jnz   ret_from_fork\n\t"
                        pushq_cfi kernel_eflags(%rip)
                        popfq_cfi                               # reset kernel eflags

--->                    irqs are still disabled

                        call schedule_tail                      # rdi: 'prev' task parameter
                                finish_lock_switch
                                        raw_spin_unlock_irq

I bet raw_spin_lock_irq at the beginning of the schedule() is set
for a reason and such change is not safe. Though I may be missing
something again...

	Cyrill

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[H. Peter Anvin]

On 07/25/2011 11:20 AM, Cyrill Gorcunov wrote:
> On Mon, Jul 25, 2011 at 02:19:02PM +0400, Cyrill Gorcunov wrote:
>> On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
>>> The following series removes the use of a global kernel_eflags variable
>>> from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
>>> 64 bit version of that code path.
>>>
>>> kernel_eflags could be made a __read_mostly but actually there is no
>>> reason to prefer the value at cpu_init() time to a compile time constant
>>> value for the initial eflags after a fork.
>>>
>>> Ian.
>>>
>>
>> Thanks, Ian! I think noone against this simplification, Peter, Andi?
>>
>> 	Cyrill
> 
> Ian, I've missed in first place that you've opened IRQs window _before_
> schedule_tail() call, ie it's not 1:1 code mapping as it was before.
> 
> Note kernel_eflags has IF clear and what we have: the ret_from_fork on
> x86-64 happens _only_ inside context_switch call, ie
> 
> schedule (sched.c)
>         ...
>         raw_spin_lock_irq
>         ...
>         context_switch
>                 switch_to
>                         "jnz   ret_from_fork\n\t"
>                         pushq_cfi kernel_eflags(%rip)
>                         popfq_cfi                               # reset kernel eflags
> 
> --->                    irqs are still disabled
> 
>                         call schedule_tail                      # rdi: 'prev' task parameter
>                                 finish_lock_switch
>                                         raw_spin_unlock_irq
> 
> I bet raw_spin_lock_irq at the beginning of the schedule() is set
> for a reason and such change is not safe. Though I may be missing
> something again...
> 

This definitely doesn't look "obviously safe" to me.  However, does
anyone see a problem with unconditionally leaving IF disabled even on 32
bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
Ian's patch #2?

	-hpa

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Cyrill Gorcunov]

On Mon, Jul 25, 2011 at 02:10:02PM -0700, H. Peter Anvin wrote:
> On 07/25/2011 11:20 AM, Cyrill Gorcunov wrote:
> > On Mon, Jul 25, 2011 at 02:19:02PM +0400, Cyrill Gorcunov wrote:
> >> On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
> >>> The following series removes the use of a global kernel_eflags variable
> >>> from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
> >>> 64 bit version of that code path.
> >>>
> >>> kernel_eflags could be made a __read_mostly but actually there is no
> >>> reason to prefer the value at cpu_init() time to a compile time constant
> >>> value for the initial eflags after a fork.
> >>>
> >>> Ian.
> >>>
> >>
> >> Thanks, Ian! I think noone against this simplification, Peter, Andi?
> >>
> >> 	Cyrill
> > 
> > Ian, I've missed in first place that you've opened IRQs window _before_
> > schedule_tail() call, ie it's not 1:1 code mapping as it was before.
> > 
> > Note kernel_eflags has IF clear and what we have: the ret_from_fork on
> > x86-64 happens _only_ inside context_switch call, ie
> > 
> > schedule (sched.c)
> >         ...
> >         raw_spin_lock_irq
> >         ...
> >         context_switch
> >                 switch_to
> >                         "jnz   ret_from_fork\n\t"
> >                         pushq_cfi kernel_eflags(%rip)
> >                         popfq_cfi                               # reset kernel eflags
> > 
> > --->                    irqs are still disabled
> > 
> >                         call schedule_tail                      # rdi: 'prev' task parameter
> >                                 finish_lock_switch
> >                                         raw_spin_unlock_irq
> > 
> > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > for a reason and such change is not safe. Though I may be missing
> > something again...
> > 
> 
> This definitely doesn't look "obviously safe" to me.  However, does
> anyone see a problem with unconditionally leaving IF disabled even on 32
> bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> Ian's patch #2?
> 
> 	-hpa
> 

On x86-32 it seems to be similar (not identical in calls though)

copy_thread()
	p->thread.ip = (unsigned long)ret_from_fork;

and the task get queued into tasks queue, but later when switch_to
happens irqs are blocked at ret_from_fork call. I better poke PeterZ
here /CC'ed/ ;)

	Cyrill

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Peter Zijlstra]

On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > schedule (sched.c)
> > >         ...
> > >         raw_spin_lock_irq
> > >         ...
> > >         context_switch
> > >                 switch_to
> > >                         "jnz   ret_from_fork\n\t"
> > >                         pushq_cfi kernel_eflags(%rip)
> > >                         popfq_cfi                               # reset kernel eflags
> > > 
> > > --->                    irqs are still disabled
> > > 
> > >                         call schedule_tail                      # rdi: 'prev' task parameter
> > >                                 finish_lock_switch
> > >                                         raw_spin_unlock_irq
> > > 
> > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > for a reason and such change is not safe. Though I may be missing
> > > something again...
> > > 
> > 
> > This definitely doesn't look "obviously safe" to me.  However, does
> > anyone see a problem with unconditionally leaving IF disabled even on 32
> > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > Ian's patch #2?

Right, enabling IRQs there isn't cool, currently there's still
__ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
that nightmare.

There's a number of very subtle things that can go wrong when you enable
interrupts over the context switch.

Leaving IRQs disabled should be the right thing, on x86 we should
_never_ have interrupts enabled there.

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Ian Campbell]

On Tue, 2011-07-26 at 16:46 +0200, Peter Zijlstra wrote:
> On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > > schedule (sched.c)
> > > >         ...
> > > >         raw_spin_lock_irq
> > > >         ...
> > > >         context_switch
> > > >                 switch_to
> > > >                         "jnz   ret_from_fork\n\t"
> > > >                         pushq_cfi kernel_eflags(%rip)
> > > >                         popfq_cfi                               # reset kernel eflags
> > > > 
> > > > --->                    irqs are still disabled
> > > > 
> > > >                         call schedule_tail                      # rdi: 'prev' task parameter
> > > >                                 finish_lock_switch
> > > >                                         raw_spin_unlock_irq
> > > > 
> > > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > > for a reason and such change is not safe. Though I may be missing
> > > > something again...
> > > > 
> > > 
> > > This definitely doesn't look "obviously safe" to me.  However, does
> > > anyone see a problem with unconditionally leaving IF disabled even on 32
> > > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > > Ian's patch #2?
> 
> Right, enabling IRQs there isn't cool, currently there's still
> __ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
> that nightmare.
> 
> There's a number of very subtle things that can go wrong when you enable
> interrupts over the context switch.
> 
> Leaving IRQs disabled should be the right thing, on x86 we should
> _never_ have interrupts enabled there.

Thanks Peter. I'll flip the (not any longer) "arbitrary" decision in my
#2 patch and repost when I return from debconf/vacation.

Ian.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Ian Campbell]

On Tue, 2011-07-26 at 15:46 +0100, Peter Zijlstra wrote:
> On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > > schedule (sched.c)
> > > >         ...
> > > >         raw_spin_lock_irq
> > > >         ...
> > > >         context_switch
> > > >                 switch_to
> > > >                         "jnz   ret_from_fork\n\t"
> > > >                         pushq_cfi kernel_eflags(%rip)
> > > >                         popfq_cfi                               # reset kernel eflags
> > > > 
> > > > --->                    irqs are still disabled
> > > > 
> > > >                         call schedule_tail                      # rdi: 'prev' task parameter
> > > >                                 finish_lock_switch
> > > >                                         raw_spin_unlock_irq
> > > > 
> > > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > > for a reason and such change is not safe. Though I may be missing
> > > > something again...
> > > > 
> > > 
> > > This definitely doesn't look "obviously safe" to me.  However, does
> > > anyone see a problem with unconditionally leaving IF disabled even on 32
> > > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > > Ian's patch #2?
> 
> Right, enabling IRQs there isn't cool, currently there's still
> __ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
> that nightmare.
> 
> There's a number of very subtle things that can go wrong when you enable
> interrupts over the context switch.
> 
> Leaving IRQs disabled should be the right thing, on x86 we should
> _never_ have interrupts enabled there.

Just getting back to this after my vac, sorry for the delay.

raw_spin_unlock_irq unconditionally re-enables interrupts so I don't
really see what I've changed since interrupts are enabled by
schedule_tail and I've moved (on 64 bit) the EFLAGS reset to after
schedule_tail, so it should have interrupts enabled at that point
already and so they should remain enabled. Or are you suggesting that
things were already wrong?

However I've switched the order of my second patch anyway, so EFLAGS is
reset to 0x0002 (interrupts disabled) on both 32- and 64-bit before the
call to schedule_tail, since it does seem like the simpler option. I'll
repost shortly.

Ian.

Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork

[Peter Zijlstra]

On Wed, 2011-08-10 at 16:27 +0100, Ian Campbell wrote:
> raw_spin_unlock_irq unconditionally re-enables interrupts so I don't
> really see what I've changed since interrupts are enabled by
> schedule_tail and I've moved (on 64 bit) the EFLAGS reset to after
> schedule_tail, so it should have interrupts enabled at that point
> already and so they should remain enabled. Or are you suggesting that
> things were already wrong?

Ah, I thought this was about the EFLAGS state of new tasks which would
get into effect at switch_to(), which is well before we enable
interrupts in finish_lock_switch().

> However I've switched the order of my second patch anyway, so EFLAGS is
> reset to 0x0002 (interrupts disabled) on both 32- and 64-bit before the
> call to schedule_tail, since it does seem like the simpler option. I'll
> repost shortly. 

OK.