Patchwork sparc, exec: remove redundant addr_limit assignment

login
register
mail settings
Submitter Mathias Krause
Date June 10, 2011, 1:10 p.m.
Message ID <1307711453-17412-1-git-send-email-minipli@googlemail.com>
Download mbox | patch
Permalink /patch/99892/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Mathias Krause - June 10, 2011, 1:10 p.m.
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(-)
David Miller - June 11, 2011, 11:08 p.m.
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
Al Viro - June 11, 2011, 11:44 p.m.
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
David Miller - June 12, 2011, 12:58 a.m.
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
Linus Torvalds - June 12, 2011, 1:01 a.m.
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
David Miller - June 12, 2011, 1:04 a.m.
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
Mathias Krause - June 13, 2011, 8:28 p.m.
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
Mathias Krause - June 17, 2011, 6:45 p.m.
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

Patch

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... */