| Message ID | 1307711453-17412-1-git-send-email-minipli@googlemail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
From: Mathias Krause <minipli@googlemail.com> Date: Fri, 10 Jun 2011 15:10:53 +0200 > The address limit is already set in flush_old_exec() so this > assignment of USER_DS is redundant. > > Signed-off-by: Mathias Krause <minipli@googlemail.com> ... > @@ -368,9 +368,6 @@ void flush_thread(void) > > /* Clear FPU register state. */ > t->fpsaved[0] = 0; > - > - if (get_thread_current_ds() != ASI_AIUS) > - set_fs(USER_DS); > } Yeah but now you're doing it unconditionally, the guard is here because the %asi register write which set_fs() does is extremely expensive on sparc64 and %99.99999 of the time we can avoid it. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 11, 2011 at 04:08:42PM -0700, David Miller wrote: > From: Mathias Krause <minipli@googlemail.com> > Date: Fri, 10 Jun 2011 15:10:53 +0200 > > > The address limit is already set in flush_old_exec() so this > > assignment of USER_DS is redundant. > > > > Signed-off-by: Mathias Krause <minipli@googlemail.com> > ... > > @@ -368,9 +368,6 @@ void flush_thread(void) > > > > /* Clear FPU register state. */ > > t->fpsaved[0] = 0; > > - > > - if (get_thread_current_ds() != ASI_AIUS) > > - set_fs(USER_DS); > > } > > Yeah but now you're doing it unconditionally, the guard is here > because the %asi register write which set_fs() does is extremely > expensive on sparc64 and %99.99999 of the time we can avoid it. OTOH, get_thread_current_ds() is cheap and moving that into set_fs() itself wouldn't be particulary bad idea... -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Al Viro <viro@ZenIV.linux.org.uk> Date: Sun, 12 Jun 2011 00:44:15 +0100 > On Sat, Jun 11, 2011 at 04:08:42PM -0700, David Miller wrote: >> From: Mathias Krause <minipli@googlemail.com> >> Date: Fri, 10 Jun 2011 15:10:53 +0200 >> >> > @@ -368,9 +368,6 @@ void flush_thread(void) >> > >> > /* Clear FPU register state. */ >> > t->fpsaved[0] = 0; >> > - >> > - if (get_thread_current_ds() != ASI_AIUS) >> > - set_fs(USER_DS); >> > } >> >> Yeah but now you're doing it unconditionally, the guard is here >> because the %asi register write which set_fs() does is extremely >> expensive on sparc64 and %99.99999 of the time we can avoid it. > > OTOH, get_thread_current_ds() is cheap and moving that into set_fs() > itself wouldn't be particulary bad idea... The reason the test is only in this specific spot, is that's the only place where the optimization makes any sense. The rest of the time it's in compat layer code or similar, where we know the set_fs() is actually necessary. Therefore, expanding the test into every set_fs() call would be wasteful. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 11, 2011 at 5:58 PM, David Miller <davem@davemloft.net> wrote: > > The reason the test is only in this specific spot, is that's the only > place where the optimization makes any sense. Well, considering that it didn't make any sense there *either*, that's kind of pointless. We always had the unconditional "set_fs()" in the exec path. It only got moved, and as part of that conscious effort, the pointless architecture churn is getting cleaned up. Linus -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Linus Torvalds <torvalds@linux-foundation.org> Date: Sat, 11 Jun 2011 18:01:06 -0700 > We always had the unconditional "set_fs()" in the exec path. It only > got moved, and as part of that conscious effort, the pointless > architecture churn is getting cleaned up. Sure. -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12.06.2011, at 01:08 David Miller wrote: > From: Mathias Krause <minipli@googlemail.com> > Date: Fri, 10 Jun 2011 15:10:53 +0200 > >> The address limit is already set in flush_old_exec() so this >> assignment of USER_DS is redundant. >> >> Signed-off-by: Mathias Krause <minipli@googlemail.com> > ... >> @@ -368,9 +368,6 @@ void flush_thread(void) >> >> /* Clear FPU register state. */ >> t->fpsaved[0] = 0; >> - >> - if (get_thread_current_ds() != ASI_AIUS) >> - set_fs(USER_DS); >> } > > Yeah but now you're doing it unconditionally, the guard is here > because the %asi register write which set_fs() does is extremely > expensive on sparc64 and %99.99999 of the time we can avoid it. As Linus already pointed out, that set_fs() was never called because we already had (and still have) an unconditional set_fs() in the arch independent code. So this patch just removes some dead code. Mathias -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12.06.2011, 01:08 David Miller wrote: > From: Mathias Krause <minipli@googlemail.com> > Date: Fri, 10 Jun 2011 15:10:53 +0200 > >> The address limit is already set in flush_old_exec() so this >> assignment of USER_DS is redundant. >> >> Signed-off-by: Mathias Krause <minipli@googlemail.com> > ... >> @@ -368,9 +368,6 @@ void flush_thread(void) >> >> /* Clear FPU register state. */ >> t->fpsaved[0] = 0; >> - >> - if (get_thread_current_ds() != ASI_AIUS) >> - set_fs(USER_DS); >> } > > Yeah but now you're doing it unconditionally, the guard is here > because the %asi register write which set_fs() does is extremely > expensive on sparc64 and %99.99999 of the time we can avoid it. David, can you please use the attached test program to give us some numbers on how expensive the write to the %asi register is, relative to the complexity of the whole exec() path. Please test it w/ and w/o the attached patch. If the difference is significant it might be worth the pain to push the set_fs() down to the arch specific code, e.g. into flush_thread() as on SPARC, and remove it from flush_old_exec(). For the test something like: $ for i in 1 2 3; do echo "run #$i:"; time exec_test 5000 /bin/true; done or better: $ perf stat -r3 exec_test 5000 /bin/true should give some reasonable numbers. I've no SPARC machine, so can't test myself. Thanks, Mathias
diff --git a/arch/sparc/kernel/process_32.c b/arch/sparc/kernel/process_32.c index c8cc461..f793742 100644 --- a/arch/sparc/kernel/process_32.c +++ b/arch/sparc/kernel/process_32.c @@ -380,8 +380,7 @@ void flush_thread(void) #endif } - /* Now, this task is no longer a kernel thread. */ - current->thread.current_ds = USER_DS; + /* This task is no longer a kernel thread. */ if (current->thread.flags & SPARC_FLAG_KTHREAD) { current->thread.flags &= ~SPARC_FLAG_KTHREAD; diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c index c158a95..d959cd0 100644 --- a/arch/sparc/kernel/process_64.c +++ b/arch/sparc/kernel/process_64.c @@ -368,9 +368,6 @@ void flush_thread(void) /* Clear FPU register state. */ t->fpsaved[0] = 0; - - if (get_thread_current_ds() != ASI_AIUS) - set_fs(USER_DS); } /* It's a bit more tricky when 64-bit tasks are involved... */
The address limit is already set in flush_old_exec() so this assignment of USER_DS is redundant. Signed-off-by: Mathias Krause <minipli@googlemail.com> --- arch/sparc/kernel/process_32.c | 3 +-- arch/sparc/kernel/process_64.c | 3 --- 2 files changed, 1 insertions(+), 5 deletions(-)