[patch] delete sync.i in ia64_switch_to()

[patch] delete sync.i in ia64_switch_to()

[Chen, Kenneth W]

The sync.i instruction in ia64_switch_to() context switch code
looks like an old leftover code.  sync.i instruction supposedly
ensures flush cache operation issued by processor become visible
to memory reference.  Everywhere I looked where fc instruction is
used (ia64_fc), a sync.i is always accompanied.  Everywhere in
the vicinity of context switch (save_switch_stack/load_switch_stack)
don't have any fc instruction in there.  I don't see any reason why
we need to issue sync.i in context switch code.  Patch to remove it.

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>


--- ./arch/ia64/kernel/entry.S.orig	2005-09-06 22:36:17.114182129 -0700
+++ ./arch/ia64/kernel/entry.S	2005-09-07 00:20:37.216644505 -0700
@@ -213,10 +213,6 @@ GLOBAL_ENTRY(ia64_switch_to)
 	mov r13=in0			// set "current" pointer
 	;;
 	DO_LOAD_SWITCH_STACK
-
-#ifdef CONFIG_SMP
-	sync.i				// ensure "fc"s done by this CPU are visible on other CPUs
-#endif
 	br.ret.sptk.many rp		// boogie on out in new context
 
 .map:

RE: [patch] delete sync.i in ia64_switch_to()

[Chen, Kenneth W]

Chen, Kenneth wrote on Wednesday, September 07, 2005 12:22 AM
> The sync.i instruction in ia64_switch_to() context switch code
> looks like an old leftover code.  sync.i instruction supposedly
> ensures flush cache operation issued by processor become visible
> to memory reference.  Everywhere I looked where fc instruction is
> used (ia64_fc), a sync.i is always accompanied.  Everywhere in
> the vicinity of context switch (save_switch_stack/load_switch_stack)
> don't have any fc instruction in there.  I don't see any reason why
> we need to issue sync.i in context switch code.  Patch to remove it.

While I'm at it, the reenabling of psr.ic should really belong to dtr
mapping code block. It make the fall through code fast since it doesn't
need to execute the predicated-off instruction.  Logically make more
sense as well since psr.ic was turned off in .map code block.

Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>


--- ./arch/ia64/kernel/entry.S.orig	2005-09-07 00:27:54.967615705 -0700
+++ ./arch/ia64/kernel/entry.S	2005-09-07 00:53:36.421698385 -0700
@@ -204,9 +204,6 @@ GLOBAL_ENTRY(ia64_switch_to)
 (p6)	br.cond.dpnt .map
 	;;
 .done:
-(p6)	ssm psr.ic			// if we had to map, reenable the psr.ic bit FIRST!!!
-	;;
-(p6)	srlz.d
 	ld8 sp=[r21]			// load kernel stack pointer of new task
 	mov IA64_KR(CURRENT)=in0	// update "current" application register
 	mov r8=r13			// return pointer to previously running task
@@ -230,6 +227,9 @@ GLOBAL_ENTRY(ia64_switch_to)
 	mov IA64_KR(CURRENT_STACK)=r26	// remember last page we mapped...
 	;;
 	itr.d dtr[r25]=r23		// wire in new mapping...
+	ssm psr.ic			// reenable the psr.ic bit
+	;;
+	srlz.d
 	br.cond.sptk .done
 END(ia64_switch_to)
 

RE: [patch] delete sync.i in ia64_switch_to()

[Jim Hull]

That sync.i is not old code - it is required.

It is not there to handle kernel fc instructions - as you say, they have
their own sync instructions.  The sync.i in the context switch path is there
to guarantee that user-mode fc's become visible.

The case it's covering is one where some user app is modifying code using
the required sequence (as described on the sync instruction page in SDM
Volume 3), but in the midst of that sequence, it gets interrupted, context
switched, and rescheduled onto some other processor.  In that case, unless
the OS issues the sync.i on the app's behalf on the original processor, then
those fc's need not ever become visible.  The sync.i that the app
(eventually) does as part of it's sequence isn't good enough, because it
executes on the second processor, and syncs aren't broadcast between
processors.

Section 4.5.2.1 of SDM 2.1, Volume 2, Part II (page 2:428) talks about this
requirement.  That section also says that an mf instruction is required in
the context switch path.

Now I'm no expert on the linux codebase, so maybe these requirements are
being met by some other part of the context-switch path.  But if not, then
rather than a patch to delete the sync instruction, I think you need one to
improve the comments, and add the missing mf.

 -- Jim
    Itanium Processor Architect at HP 

> -----Original Message-----
> From: linux-ia64-owner@vger.kernel.org 
> [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of Chen, Kenneth W
> Sent: Wednesday, September 07, 2005 12:22 AM
> To: linux-ia64@vger.kernel.org
> Subject: [patch] delete sync.i in ia64_switch_to()
> 
> The sync.i instruction in ia64_switch_to() context switch code
> looks like an old leftover code.  sync.i instruction supposedly
> ensures flush cache operation issued by processor become visible
> to memory reference.  Everywhere I looked where fc instruction is
> used (ia64_fc), a sync.i is always accompanied.  Everywhere in
> the vicinity of context switch (save_switch_stack/load_switch_stack)
> don't have any fc instruction in there.  I don't see any reason why
> we need to issue sync.i in context switch code.  Patch to remove it.
> 
> Signed-off-by: Ken Chen <kenneth.w.chen@intel.com>
> 
> 
> --- ./arch/ia64/kernel/entry.S.orig	2005-09-06 
> 22:36:17.114182129 -0700
> +++ ./arch/ia64/kernel/entry.S	2005-09-07 
> 00:20:37.216644505 -0700
> @@ -213,10 +213,6 @@ GLOBAL_ENTRY(ia64_switch_to)
>  	mov r13=in0			// set "current" pointer
>  	;;
>  	DO_LOAD_SWITCH_STACK
> -
> -#ifdef CONFIG_SMP
> -	sync.i				// ensure "fc"s done by 
> this CPU are visible on other CPUs
> -#endif
>  	br.ret.sptk.many rp		// boogie on out in new context
>  
>  .map:
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-ia64" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] delete sync.i in ia64_switch_to()

[david mosberger]

On 9/7/05, Jim Hull <jim.hull@hp.com> wrote:
> That sync.i is not old code - it is required.

Yes, the sync.i was added by Asit at some point precisely for the
reasons you explained.  I believe we concluded at the time that the
"mf" wasn't needed since we'd be guaranteed to have a fence in the
execution-path anyhow (IIRC, it was due to locks being
acquired/released on the paths in question).  I'm not sure, however,
whether this point has been considered when kernel-preemption support
was added.  The folks caring about CONFIG_PREEMPT may want to
reconsider that issue if not.

  --david

RE: [patch] delete sync.i in ia64_switch_to()

[Chen, Kenneth W]

Jim Hull wrote on Wednesday, September 07, 2005 10:33 AM
> That sync.i is not old code - it is required.
> 
> It is not there to handle kernel fc instructions - as you say, they have
> their own sync instructions.  The sync.i in the context switch path is there
> to guarantee that user-mode fc's become visible.

Thank you for the explanation.  Now I see the lights :-)

- Ken