Hi Anshuman,
On Wed, Mar 2, 2022 at 12:07 PM Anshuman Khandual
<anshuman.khandual@xxxxxxx> wrote:
On 3/2/22 3:35 PM, Geert Uytterhoeven wrote:
On Wed, Mar 2, 2022 at 10:51 AM Anshuman Khandual
<anshuman.khandual@xxxxxxx> wrote:
On 3/2/22 12:35 PM, Christophe Leroy wrote:
Le 02/03/2022 à 04:22, Anshuman Khandual a écrit :
On 3/1/22 1:46 PM, Christophe Leroy wrote:
Le 01/03/2022 à 01:31, Russell King (Oracle) a écrit :
On Tue, Mar 01, 2022 at 05:30:41AM +0530, Anshuman Khandual wrote:
On 2/28/22 4:27 PM, Russell King (Oracle) wrote:
On Mon, Feb 28, 2022 at 04:17:32PM +0530, Anshuman Khandual wrote:
This defines and exports a platform specific custom vm_get_page_prot() via
subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX
macros can be dropped which are no longer needed.
What I would really like to know is why having to run _code_ to work out
what the page protections need to be is better than looking it up in a
table.
Not only is this more expensive in terms of CPU cycles, it also brings
additional code size with it.
I'm struggling to see what the benefit is.
Currently vm_get_page_prot() is also being _run_ to fetch required page
protection values. Although that is being run in the core MM and from a
platform perspective __SXXX, __PXXX are just being exported for a table.
Looking it up in a table (and applying more constructs there after) is
not much different than a clean switch case statement in terms of CPU
usage. So this is not more expensive in terms of CPU cycles.
I disagree.
So do I.
However, let's base this disagreement on some evidence. Here is the
present 32-bit ARM implementation:
00000048 <vm_get_page_prot>:
48: e200000f and r0, r0, #15
4c: e3003000 movw r3, #0
4c: R_ARM_MOVW_ABS_NC .LANCHOR1
50: e3403000 movt r3, #0
50: R_ARM_MOVT_ABS .LANCHOR1
54: e7930100 ldr r0, [r3, r0, lsl #2]
58: e12fff1e bx lr
That is five instructions long.
On ppc32 I get:
00000094 <vm_get_page_prot>:
94: 3d 20 00 00 lis r9,0
96: R_PPC_ADDR16_HA .data..ro_after_init
98: 54 84 16 ba rlwinm r4,r4,2,26,29
9c: 39 29 00 00 addi r9,r9,0
9e: R_PPC_ADDR16_LO .data..ro_after_init
a0: 7d 29 20 2e lwzx r9,r9,r4
a4: 91 23 00 00 stw r9,0(r3)
a8: 4e 80 00 20 blr
Please show that your new implementation is not more expensive on
32-bit ARM. Please do so by building a 32-bit kernel, and providing
the disassembly.
With your series I get:
00000000 <vm_get_page_prot>:
0: 3d 20 00 00 lis r9,0
2: R_PPC_ADDR16_HA .rodata
4: 39 29 00 00 addi r9,r9,0
6: R_PPC_ADDR16_LO .rodata
8: 54 84 16 ba rlwinm r4,r4,2,26,29
c: 7d 49 20 2e lwzx r10,r9,r4
10: 7d 4a 4a 14 add r10,r10,r9
14: 7d 49 03 a6 mtctr r10
18: 4e 80 04 20 bctr
1c: 39 20 03 15 li r9,789
20: 91 23 00 00 stw r9,0(r3)
24: 4e 80 00 20 blr
28: 39 20 01 15 li r9,277
2c: 91 23 00 00 stw r9,0(r3)
30: 4e 80 00 20 blr
34: 39 20 07 15 li r9,1813
38: 91 23 00 00 stw r9,0(r3)
3c: 4e 80 00 20 blr
40: 39 20 05 15 li r9,1301
44: 91 23 00 00 stw r9,0(r3)
48: 4e 80 00 20 blr
4c: 39 20 01 11 li r9,273
50: 4b ff ff d0 b 20 <vm_get_page_prot+0x20>
That is definitely more expensive, it implements a table of branches.
Okay, will split out the PPC32 implementation that retains existing
table look up method. Also planning to keep that inside same file
(arch/powerpc/mm/mmap.c), unless you have a difference preference.
My point was not to get something specific for PPC32, but to amplify on
Russell's objection.
As this is bad for ARM and bad for PPC32, do we have any evidence that
your change is good for any other architecture ?
I checked PPC64 and there is exactly the same drawback. With the current
implementation it is a small function performing table read then a few
adjustment. After your change it is a bigger function implementing a
table of branches.
I am wondering if this would not be the case for any other switch case
statement on the platform ? Is there something specific/different just
on vm_get_page_prot() implementation ? Are you suggesting that switch
case statements should just be avoided instead ?
So, as requested by Russell, could you look at the disassembly for other
architectures and show us that ARM and POWERPC are the only ones for
which your change is not optimal ?
But the primary purpose of this series is not to guarantee optimized
code on platform by platform basis, while migrating from a table based
look up method into a switch case statement.
But instead, the purposes is to remove current levels of unnecessary
abstraction while converting a vm_flags access combination into page
protection. The switch case statement for platform implementation of
vm_get_page_prot() just seemed logical enough. Christoph's original
suggestion patch for x86 had the same implementation as well.
But if the table look up is still better/preferred method on certain
platforms like arm or ppc32, will be happy to preserve that.
I doubt the switch() variant would give better code on any platform.
What about using tables everywhere, using designated initializers
to improve readability?
Designated initializers ? Could you please be more specific. A table look
up on arm platform would be something like this and arm_protection_map[]
needs to be updated with user_pgprot like before. Just wondering how a
designated initializer will help here.
It's more readable than the original:
pgprot_t protection_map[16] __ro_after_init = {
__P000, __P001, __P010, __P011, __P100, __P101, __P110, __P111,
__S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111
};
static pgprot_t arm_protection_map[16] __ro_after_init = {
[VM_NONE] = __PAGE_NONE,
[VM_READ] = __PAGE_READONLY,
[VM_WRITE] = __PAGE_COPY,
[VM_WRITE | VM_READ] = __PAGE_COPY,
[VM_EXEC] = __PAGE_READONLY_EXEC,
[VM_EXEC | VM_READ] = __PAGE_READONLY_EXEC,
[VM_EXEC | VM_WRITE] = __PAGE_COPY_EXEC,
[VM_EXEC | VM_WRITE | VM_READ] = __PAGE_COPY_EXEC,
[VM_SHARED] = __PAGE_NONE,
[VM_SHARED | VM_READ] = __PAGE_READONLY,
[VM_SHARED | VM_WRITE] = __PAGE_SHARED,
[VM_SHARED | VM_WRITE | VM_READ] = __PAGE_SHARED,
[VM_SHARED | VM_EXEC] = __PAGE_READONLY_EXEC,
[VM_SHARED | VM_EXEC | VM_READ] = __PAGE_READONLY_EXEC,
[VM_SHARED | VM_EXEC | VM_WRITE] = __PAGE_SHARED_EXEC,
[VM_SHARED | VM_EXEC | VM_WRITE | VM_READ] = __PAGE_SHARED_EXEC
};
Yeah, like that.
Seems like you already made such a conversion in
https://lore.kernel.org/all/1645425519-9034-3-git-send-email-anshuman.khandual@xxxxxxx/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
