[PATCH 2/2] powerpc/spufs: stop using access_ok

[Christophe Leroy]

Le 30/04/2020 à 07:39, Christoph Hellwig a écrit :
> On Thu, Apr 30, 2020 at 08:39:00AM +0800, Jeremy Kerr wrote:
>> Hi Christophe,
>>
>>>> Just use the proper non __-prefixed get/put_user variants where
>>>> that is not done yet.
>>>
>>> But it means you are doing the access_ok() check everytime, which is
>>> what is to be avoided by doing the access_ok() once then using the
>>> __-prefixed variant.
>>
>> 5 out of 8 of these are just a access_ok(); simple_read_from_buffer().
>>
>> For the cases where it's multiple __put/get_user()s, the max will be 5.
>> (for the mbox access). Is that worth optimising the access_ok() checks?
> 
> access_ok is just trivial comparism to the segment limit, I don't
> think it has a relavant performance impact.
> 

I think it has an impact. See the difference between the two following 
trivial functions:

int test1(unsigned long val, unsigned long *addr)
{
	return __put_user(val, addr);
}

int test2(unsigned long val, unsigned long *addr)
{
	return put_user(val, addr);
}


00000000 <test1>:
    0:	39 20 00 00 	li      r9,0
    4:	90 64 00 00 	stw     r3,0(r4)
    8:	7d 23 4b 78 	mr      r3,r9
    c:	4e 80 00 20 	blr

00000000 <test2>:
    0:	81 22 04 38 	lwz     r9,1080(r2)
    4:	7c 6a 1b 78 	mr      r10,r3
    8:	7f 89 20 40 	cmplw   cr7,r9,r4
    c:	41 9c 00 24 	blt     cr7,30 <test2+0x30>
   10:	7d 24 48 50 	subf    r9,r4,r9
   14:	38 60 ff f2 	li      r3,-14
   18:	2b 89 00 02 	cmplwi  cr7,r9,2
   1c:	4c 9d 00 20 	blelr   cr7
   20:	39 20 00 00 	li      r9,0
   24:	91 44 00 00 	stw     r10,0(r4)
   28:	7d 23 4b 78 	mr      r3,r9
   2c:	4e 80 00 20 	blr
   30:	38 60 ff f2 	li      r3,-14
   34:	4e 80 00 20 	blr


It looks like GCC is smart enough to read the limit in task struct only 
once when we have two consecutive put_user() but there is still some 
difference:

int test3(unsigned long val, unsigned long *addr)
{
	return put_user(val, addr) ? : put_user(val, addr + 1);
}

int test4(unsigned long val, unsigned long *addr)
{
	if (!access_ok(addr, sizeof(*addr)))
		return -EFAULT;

	return __put_user(val, addr) ? : __put_user(val, addr + 1);
}

00000000 <test3>:
    0:	81 42 04 38 	lwz     r10,1080(r2)
    4:	7f 8a 20 40 	cmplw   cr7,r10,r4
    8:	41 9c 00 48 	blt     cr7,50 <test3+0x50>
    c:	7d 04 50 50 	subf    r8,r4,r10
   10:	39 20 ff f2 	li      r9,-14
   14:	2b 88 00 02 	cmplwi  cr7,r8,2
   18:	40 9d 00 30 	ble     cr7,48 <test3+0x48>
   1c:	39 20 00 00 	li      r9,0
   20:	90 64 00 00 	stw     r3,0(r4)
   24:	2f 89 00 00 	cmpwi   cr7,r9,0
   28:	40 9e 00 20 	bne     cr7,48 <test3+0x48>
   2c:	38 84 00 04 	addi    r4,r4,4
   30:	7f 8a 20 40 	cmplw   cr7,r10,r4
   34:	41 9c 00 1c 	blt     cr7,50 <test3+0x50>
   38:	7d 44 50 50 	subf    r10,r4,r10
   3c:	2b 8a 00 02 	cmplwi  cr7,r10,2
   40:	40 9d 00 10 	ble     cr7,50 <test3+0x50>
   44:	90 64 00 00 	stw     r3,0(r4)
   48:	7d 23 4b 78 	mr      r3,r9
   4c:	4e 80 00 20 	blr
   50:	39 20 ff f2 	li      r9,-14
   54:	4b ff ff f4 	b       48 <test3+0x48>

Disassembly of section .text.test4:

00000000 <test4>:
    0:	81 22 04 38 	lwz     r9,1080(r2)
    4:	7f 89 20 40 	cmplw   cr7,r9,r4
    8:	41 9c 00 34 	blt     cr7,3c <test4+0x3c>
    c:	7d 44 48 50 	subf    r10,r4,r9
   10:	39 20 ff f2 	li      r9,-14
   14:	2b 8a 00 06 	cmplwi  cr7,r10,6
   18:	40 9d 00 1c 	ble     cr7,34 <test4+0x34>
   1c:	39 20 00 00 	li      r9,0
   20:	90 64 00 00 	stw     r3,0(r4)
   24:	2f 89 00 00 	cmpwi   cr7,r9,0
   28:	40 9e 00 0c 	bne     cr7,34 <test4+0x34>
   2c:	38 84 00 04 	addi    r4,r4,4
   30:	90 64 00 00 	stw     r3,0(r4)
   34:	7d 23 4b 78 	mr      r3,r9
   38:	4e 80 00 20 	blr
   3c:	39 20 ff f2 	li      r9,-14
   40:	4b ff ff f4 	b       34 <test4+0x34>