add likely around access_ok for uaccess

PATCH In review Manfred Spraul 5 participants 10 messages Sep 14, 2003 – Sep 17, 2003

lore

Share & tools

lore

Manfred Spraul

author

2003-09-14 11:17 UTC #0 lore raw

PATCH 1/1 add likely around access_ok for uaccess

Hi,

while trying to figure out why sysv msg is around 30% slower than pipes for data transfers I noticed that gcc's autodetection (3.2.2) guesses the "if(access_ok())" tests in uaccess.h wrong and puts the error memset into the direct path and the copy out of line.

The attached patch adds likely to the tests - any objections? What about the archs except i386?

--
Manfred

include/asm-i386/uaccess.h +3 -3

@@ -300,7 +300,7 @@
 	long __pu_err = -EFAULT;					\
 	__typeof__(*(ptr)) *__pu_addr = (ptr);				\
 	might_sleep();						\
-	if (access_ok(VERIFY_WRITE,__pu_addr,size))			\
+	if (likely(access_ok(VERIFY_WRITE,__pu_addr,size)))			\
 		__put_user_size((x),__pu_addr,(size),__pu_err,-EFAULT);	\
 	__pu_err;							\
 })

@@ -510,7 +510,7 @@
 direct_copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	might_sleep();
-	if (access_ok(VERIFY_WRITE, to, n))
+	if (likely(access_ok(VERIFY_WRITE, to, n)))
 		n = __direct_copy_to_user(to, from, n);
 	return n;
 }

@@ -535,7 +535,7 @@
 direct_copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	might_sleep();
-	if (access_ok(VERIFY_READ, from, n))
+	if (likely(access_ok(VERIFY_READ, from, n)))
 		n = __direct_copy_from_user(to, from, n);
 	else
 		memset(to, 0, n);

Andrew Morton

2003-09-14 19:30 UTC #1 lore raw

Quoted 6 lines · Manfred Spraul

I noticed that gcc's autodetection (3.2.2) guesses
the "if(access_ok())" tests in uaccess.h wrong and puts the error memset into the direct path and the copy out of line.
The attached patch adds likely to the tests

Fair enough. There are quite a few other access_ok() callers. How
about just:

include/asm-i386/uaccess.h renamed from include/asm-i386/uaccess.h~access_ok-is-likely +1 -1

@@ -80,7 +80,7 @@ extern struct movsl_mask {
  * checks that the pointer is in the user space range - after calling
  * this function, memory access functions may still return -EFAULT.
  */
-#define access_ok(type,addr,size) (__range_ok(addr,size) == 0)
+#define access_ok(type,addr,size) (likely(__range_ok(addr,size) == 0))
 
 /**
  * verify_area: - Obsolete, use access_ok()

Manfred Spraul

author

2003-09-14 19:45 UTC #2 lore raw

Quoted 3 lines · Andrew Morton

+#define access_ok(type,addr,size) (likely(__range_ok(addr,size) == 0))

I don't know. What happens if access_ok is used outside of an if statement? One example is

int get_compat_timespec(struct timespec *ts, struct compat_timespec *cts)

{
       return (verify_area(VERIFY_READ, cts, sizeof(*cts)) ||
                       __get_user(ts->tv_sec, &cts->tv_sec) ||
                       __get_user(ts->tv_nsec, &cts->tv_nsec)) ? -EFAULT : 0;
}

And fs/reiserfs/file.c already contains an unlikely marker around access_ok.

Have you tried if the kernel still compiles with your patch?

--
Manfred

Manfred Spraul

author

2003-09-14 19:49 UTC #3 lore raw

Quoted 8 lines · Manfred Spraul

Quoted 3 lines · Andrew Morton
+#define access_ok(type,addr,size) (likely(__range_ok(addr,size) == 0))
I don't know. What happens if access_ok is used outside of an if statement? One example is

Quoted 7 lines

int get_compat_timespec(struct timespec *ts, struct compat_timespec *cts)
{
       return (verify_area(VERIFY_READ, cts, sizeof(*cts)) ||
                       __get_user(ts->tv_sec, &cts->tv_sec) ||
                       __get_user(ts->tv_nsec, &cts->tv_nsec)) ? 
-EFAULT : 0;
}

Duh. Reading is difficult.
The functions with access_ok are get_compat_itimerval and put_compat_itimerval.

--
Manfred

Andrew Morton

2003-09-14 20:00 UTC #4 lore raw

In reply to Manfred Spraul

Quoted 2 lines · Manfred Spraul

What happens if access_ok is used outside of an if statement?

Seems OK. Even this compiles:

        int x;

        x = likely(y == 0);

David S. Miller

2003-09-15 23:12 UTC #5 lore raw

In reply to Manfred Spraul

Quoted 2 lines · On Sun, 14 Sep 2003 13:17:10 +0200 Manfred Spraul <[email protected]>…

The attached patch adds likely to the tests - any objections? What about the archs except i386?

On sparc64, access_ok() is a constant, so it doesn't matter there.

Sparc32 probably should have it.

Pavel Machek

2003-09-16 19:49 UTC #6 lore raw

In reply to Manfred Spraul

Hi!

Quoted 7 lines

while trying to figure out why sysv msg is around 30% slower than pipes for data transfers I noticed that gcc's autodetection (3.2.2) guesses the "if(access_ok())" tests in uaccess.h wrong and puts the error memset into the direct path and the copy out of line.
The attached patch adds likely to the tests - any objections? What about the archs except i386?

How much speedup did you gain?
Pavel

Signature · 2 lines

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Jeff Garzik

2003-09-16 20:55 UTC #7 lore raw

Quoted 11 lines · Pavel Machek

Hi!
Quoted 7 lines
while trying to figure out why sysv msg is around 30% slower than pipes for data transfers I noticed that gcc's autodetection (3.2.2) guesses the "if(access_ok())" tests in uaccess.h wrong and puts the error memset into the direct path and the copy out of line.
The attached patch adds likely to the tests - any objections? What about the archs except i386?
How much speedup did you gain?

How much can it hurt?

Jeff

Pavel Machek

2003-09-16 20:59 UTC #8 lore raw

Hi!

Quoted 11 lines

Quoted 9 lines
Quoted 7 lines
while trying to figure out why sysv msg is around 30% slower than pipes for data transfers I noticed that gcc's autodetection (3.2.2) guesses the "if(access_ok())" tests in uaccess.h wrong and puts the error memset into the direct path and the copy out of line.
The attached patch adds likely to the tests - any objections? What about the archs except i386?
How much speedup did you gain?
How much can it hurt?

The change is obviously okay, I just wanted to know... If it gains 30% on sysv messages.. that would be pretty big surprise.
Pavel

Signature · 2 lines

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Manfred Spraul

author

2003-09-17 16:57 UTC #9 lore raw

Quoted 12 lines · Pavel Machek

Hi!
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
while trying to figure out why sysv msg is around 30% slower than pipes for data transfers I noticed that gcc's autodetection (3.2.2) guesses the "if(access_ok())" tests in uaccess.h wrong and puts the error memset into the direct path and the copy out of line.
The attached patch adds likely to the tests - any objections? What about the archs except i386?
How much speedup did you gain?

Quoted 7 lines

Quoted 3 lines
How much can it hurt?
The change is obviously okay, I just wanted to know... If it gains 30% on sysv messages.. that would be pretty big surprise.

No.
I didn't benchmark it at all. I'd expect one or two cycles.
The 30% difference are due to misaligned buffers: The sysvmsg ABI uses
struct msgbuf {
unsigned long type;
char data[];
};
msgbuf was aligned, thus data on an not 8-byte aligned address. Thus the "rep;movsl" microcode fastpath didn't kick in, and that caused the 30-50% slowdown.
After manually misaligning the msgbuf structure, and properly aligning the kernel buffers, the performance of sysvmsg is now identical to pipes: Around 4 k cycles for a one-byte ping-pong, and around 10k cycles for 4 kB ping-pong, with a Celeron mobile 1.13 GHz.

I haven't decided yet if a patch to align the kernel buffers is a good thing or not - it only helps my benchmark, I'm not aware of a real-world app that uses sysvmsg for bulk data transfers.

--
Manfred