From: Nicholas Piggin <npiggin@gmail.com> Date: 2016-07-22 12:57:49
Calculating the slice mask can become a signifcant overhead for
get_unmapped_area. The mask is relatively small and does not change
frequently, so we can cache it in the mm context.
This saves about 30% kernel time on a 4K user address allocation
in a microbenchmark.
Comments on the approach taken? I think there is the option for fixed
allocations to avoid some of the slice calculation entirely, but first
I think it will be good to have a general speedup that covers all
mmaps.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Anton Blanchard <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 8 +++++++
arch/powerpc/mm/slice.c | 39 ++++++++++++++++++++++++++++++--
2 files changed, 45 insertions(+), 2 deletions(-)
On Fri, Jul 22, 2016 at 10:57:28PM +1000, Nicholas Piggin wrote:
quoted hunk
Calculating the slice mask can become a signifcant overhead for
get_unmapped_area. The mask is relatively small and does not change
frequently, so we can cache it in the mm context.
This saves about 30% kernel time on a 4K user address allocation
in a microbenchmark.
Comments on the approach taken? I think there is the option for fixed
allocations to avoid some of the slice calculation entirely, but first
I think it will be good to have a general speedup that covers all
mmaps.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Anton Blanchard <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 8 +++++++
arch/powerpc/mm/slice.c | 39 ++++++++++++++++++++++++++++++--
2 files changed, 45 insertions(+), 2 deletions(-)
Should we cache these in mmu_psize_defs? I am not 100% sure
if want to overload that structure, but it provides a convient
way of saying mmu_psize_defs[psize].mask instead of all
the if checks
@@ -171,6 +171,36 @@ static struct slice_mask slice_mask_for_size(struct mm_struct *mm, int psize)returnret;}+staticvoidrecalc_slice_mask_cache(structmm_struct*mm)+{+mm->context.mask_4k=calc_slice_mask_for_size(mm,MMU_PAGE_4K);+#ifdef CONFIG_PPC_64K_PAGES+mm->context.mask_64k=calc_slice_mask_for_size(mm,MMU_PAGE_64K);+#endif+# ifdef CONFIG_HUGETLB_PAGE+/* Radix does not come here */+mm->context.mask_16m=calc_slice_mask_for_size(mm,MMU_PAGE_16M);+mm->context.mask_16g=calc_slice_mask_for_size(mm,MMU_PAGE_16G);+# endif+}
Should the function above be called under slice_convert_lock?
On Fri, Jul 22, 2016 at 10:57:28PM +1000, Nicholas Piggin wrote:
quoted
Calculating the slice mask can become a signifcant overhead for
get_unmapped_area. The mask is relatively small and does not change
frequently, so we can cache it in the mm context.
This saves about 30% kernel time on a 4K user address allocation
in a microbenchmark.
Comments on the approach taken? I think there is the option for
fixed allocations to avoid some of the slice calculation entirely,
but first I think it will be good to have a general speedup that
covers all mmaps.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Anton Blanchard <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 8 +++++++
arch/powerpc/mm/slice.c | 39
++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+),
2 deletions(-)
Should we cache these in mmu_psize_defs? I am not 100% sure
if want to overload that structure, but it provides a convient
way of saying mmu_psize_defs[psize].mask instead of all
the if checks
I'm not sure if we can, can we? mmu_psize_defs is global
whereas we need per-process structure.
The branches are a bit annoying, but we can't directly use an array
because it's too big. But see the comment at MMU_PAGE_* defines.
Perhaps we could change this structure to be sized at compile time to
only include possible page sizes, and would enable building a
structure like the above with simply
struct type blah[MMU_POSSIBLE_PAGE_COUNT];
Perhaps we can consider that as a follow on patch? It's probably a bit
more work to implement.
Should the function above be called under slice_convert_lock?
Good question. The slice_convert_lock is... interesting. It only
protects the update-side of the slice page size arrays. I thought
this was okay last time I looked, but now you make me think again
maybe it is not. I need to check again what's providing exclusion
on the read side too.
I wanted to avoid doing more work under slice_convert_lock, but
we should just make that a per-mm lock anyway shouldn't we?
Thanks,
Nick
On Fri, Jul 22, 2016 at 10:57:28PM +1000, Nicholas Piggin wrote:
quoted
Calculating the slice mask can become a signifcant overhead for
get_unmapped_area. The mask is relatively small and does not change
frequently, so we can cache it in the mm context.
This saves about 30% kernel time on a 4K user address allocation
in a microbenchmark.
Comments on the approach taken? I think there is the option for
fixed allocations to avoid some of the slice calculation entirely,
but first I think it will be good to have a general speedup that
covers all mmaps.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Anton Blanchard <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 8 +++++++
arch/powerpc/mm/slice.c | 39
++++++++++++++++++++++++++++++-- 2 files changed, 45 insertions(+),
2 deletions(-)
Should we cache these in mmu_psize_defs? I am not 100% sure
if want to overload that structure, but it provides a convient
way of saying mmu_psize_defs[psize].mask instead of all
the if checks
I'm not sure if we can, can we? mmu_psize_defs is global
whereas we need per-process structure.
Oh! sorry, I meant a structure like mmu_psize_defs.
The branches are a bit annoying, but we can't directly use an array
because it's too big. But see the comment at MMU_PAGE_* defines.
Perhaps we could change this structure to be sized at compile time to
only include possible page sizes, and would enable building a
structure like the above with simply
struct type blah[MMU_POSSIBLE_PAGE_COUNT];
Perhaps we can consider that as a follow on patch? It's probably a bit
more work to implement.
Yeah.. good idea
MMU_PAGE_COUNT is 15, the size is going to be 15*8 bytes?
Should the function above be called under slice_convert_lock?
Good question. The slice_convert_lock is... interesting. It only
protects the update-side of the slice page size arrays. I thought
this was okay last time I looked, but now you make me think again
maybe it is not. I need to check again what's providing exclusion
on the read side too.
I wanted to avoid doing more work under slice_convert_lock, but
we should just make that a per-mm lock anyway shouldn't we?
Yeah and Ben's comment in the reply suggest we already hold a
per mm lock on the read side.
Balbir Singh
From: Nicholas Piggin <npiggin@gmail.com> Date: 2016-07-25 02:28:09
On Sat, 23 Jul 2016 18:49:06 +1000
Benjamin Herrenschmidt [off-list ref] wrote:
On Sat, 2016-07-23 at 17:10 +1000, Nicholas Piggin wrote:
quoted
I wanted to avoid doing more work under slice_convert_lock, but
we should just make that a per-mm lock anyway shouldn't we?
Aren't the readers under the mm sem taken for writing or has this
changed ?
I don't think this has changed, but I look at the writers now,
which aren't synchronized by mm sem. But neither are readers
under the slice_convert_lock, so I'm looking at what the locking
actually is. Is it just using atomicity of dword stores vs loads?
Thanks,
Nick
On Fri, Jul 22, 2016 at 10:57:28PM +1000, Nicholas Piggin wrote:
quoted
Calculating the slice mask can become a signifcant overhead for
get_unmapped_area. The mask is relatively small and does not
change frequently, so we can cache it in the mm context.
This saves about 30% kernel time on a 4K user address allocation
in a microbenchmark.
Comments on the approach taken? I think there is the option for
fixed allocations to avoid some of the slice calculation
entirely, but first I think it will be good to have a general
speedup that covers all mmaps.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Anton Blanchard <redacted>
---
arch/powerpc/include/asm/book3s/64/mmu.h | 8 +++++++
arch/powerpc/mm/slice.c | 39
++++++++++++++++++++++++++++++-- 2 files changed, 45
insertions(+), 2 deletions(-)
Should we cache these in mmu_psize_defs? I am not 100% sure
if want to overload that structure, but it provides a convient
way of saying mmu_psize_defs[psize].mask instead of all
the if checks
I'm not sure if we can, can we? mmu_psize_defs is global
whereas we need per-process structure.
Oh! sorry, I meant a structure like mmu_psize_defs.
In that case, sure. Avoiding the branches might be worthwhile.
quoted
The branches are a bit annoying, but we can't directly use an array
because it's too big. But see the comment at MMU_PAGE_* defines.
Perhaps we could change this structure to be sized at compile time
to only include possible page sizes, and would enable building a
structure like the above with simply
struct type blah[MMU_POSSIBLE_PAGE_COUNT];
Perhaps we can consider that as a follow on patch? It's probably a
bit more work to implement.
Yeah.. good idea
MMU_PAGE_COUNT is 15, the size is going to be 15*8 bytes?
Unfortunately, slice_mask is 16 bytes. Only 10 are used, but it
seemed too ugly to try squashing things together.
quoted
Good question. The slice_convert_lock is... interesting. It only
protects the update-side of the slice page size arrays. I thought
this was okay last time I looked, but now you make me think again
maybe it is not. I need to check again what's providing exclusion
on the read side too.
I wanted to avoid doing more work under slice_convert_lock, but
we should just make that a per-mm lock anyway shouldn't we?
Yeah and Ben's comment in the reply suggest we already hold a
per mm lock on the read side.
Let's discuss this further in my reply to Ben.
Thanks,
Nick