Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
V4 Changes:
Replaced the BUILD_BUG_ON with VM_BUG_ON.
Moved fault_around_pages() and fault_around_mask() functions outside of
#ifdef CONFIG_DEBUG_FS.
V3 Changes:
Replaced FAULT_AROUND_ORDER macro to a variable to support arch's that
supports sub platforms.
Made changes in commit messages.
V2 Changes:
Created Kconfig parameter for FAULT_AROUND_ORDER
Added check in do_read_fault to handle FAULT_AROUND_ORDER value of 0
Made changes in commit messages.
Madhavan Srinivasan (2):
mm: move FAULT_AROUND_ORDER to arch/
powerpc/pseries: init fault_around_order for pseries
arch/powerpc/platforms/pseries/pseries.h | 2 ++
arch/powerpc/platforms/pseries/setup.c | 5 +++++
mm/Kconfig | 8 ++++++++
mm/memory.c | 25 ++++++-------------------
4 files changed, 21 insertions(+), 19 deletions(-)
--
1.7.10.4
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. Patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4.
Signed-off-by: Madhavan Srinivasan <redacted>
---
mm/Kconfig | 8 ++++++++
mm/memory.c | 25 ++++++-------------------
2 files changed, 14 insertions(+), 19 deletions(-)
@@ -176,6 +176,14 @@ config MOVABLE_NODEconfigHAVE_BOOTMEM_INFO_NODEdef_booln+#+# Fault around order is a control knob to decide the fault around pages.+# Default value is set to 4 , but the arch can override it as desired.+#+configFAULT_AROUND_ORDER+int+default4+# eventually, we can have this option just 'select SPARSEMEM'configMEMORY_HOTPLUGbool"Allow for memory hot-add"
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
V4 Changes:
Replaced the BUILD_BUG_ON with VM_BUG_ON.
Moved fault_around_pages() and fault_around_mask() functions outside of
#ifdef CONFIG_DEBUG_FS.
V3 Changes:
Replaced FAULT_AROUND_ORDER macro to a variable to support arch's that
supports sub platforms.
Made changes in commit messages.
V2 Changes:
Created Kconfig parameter for FAULT_AROUND_ORDER
Added check in do_read_fault to handle FAULT_AROUND_ORDER value of 0
Made changes in commit messages.
Madhavan Srinivasan (2):
mm: move FAULT_AROUND_ORDER to arch/
powerpc/pseries: init fault_around_order for pseries
arch/powerpc/platforms/pseries/pseries.h | 2 ++
arch/powerpc/platforms/pseries/setup.c | 5 +++++
mm/Kconfig | 8 ++++++++
mm/memory.c | 25 ++++++-------------------
4 files changed, 21 insertions(+), 19 deletions(-)
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
quoted
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
Sorry for not commenting earlier - just reminded by this ping to Ingo.
I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
arch/powerpc/Kconfig suggests that Power supports base page size of
4k, 16k, 64k or 256k.
I would expect your optimal fault_around_order to depend very much on
the base page size.
Perhaps fault_around_size would provide a more useful default?
Hugh
From: Rusty Russell <hidden> Date: 2014-05-19 01:43:50
Hugh Dickins [off-list ref] writes:
On Thu, 15 May 2014, Madhavan Srinivasan wrote:
quoted
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
quoted
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
Sorry for not commenting earlier - just reminded by this ping to Ingo.
I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
arch/powerpc/Kconfig suggests that Power supports base page size of
4k, 16k, 64k or 256k.
I would expect your optimal fault_around_order to depend very much on
the base page size.
It was 64k, which is what PPC64 uses on all the major distributions.
You really only get a choice of 4k and 64k with 64 bit power.
Perhaps fault_around_size would provide a more useful default?
That seems to fit. With 4k pages and order 4, you're asking for 64k.
Maddy's result shows 64k is also reasonable for 64k pages.
Perhaps we try to generalize from two data points (a slight improvement
over doing it from 1!), eg:
/* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
unsigned int fault_around_order __read_mostly =
(16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
Cheers,
Rusty.
On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
Hugh Dickins [off-list ref] writes:
quoted
On Thu, 15 May 2014, Madhavan Srinivasan wrote:
quoted
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
quoted
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
Sorry for not commenting earlier - just reminded by this ping to Ingo.
I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
arch/powerpc/Kconfig suggests that Power supports base page size of
4k, 16k, 64k or 256k.
I would expect your optimal fault_around_order to depend very much on
the base page size.
It was 64k, which is what PPC64 uses on all the major distributions.
You really only get a choice of 4k and 64k with 64 bit power.
This is true. PPC64 support multiple pagesize and yes the default page
size of 64k, is taken as base pagesize for the tests.
quoted
Perhaps fault_around_size would provide a more useful default?
That seems to fit. With 4k pages and order 4, you're asking for 64k.
Maddy's result shows 64k is also reasonable for 64k pages.
Perhaps we try to generalize from two data points (a slight improvement
over doing it from 1!), eg:
/* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
unsigned int fault_around_order __read_mostly =
(16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
This may be right. But these are the concerns, will not this make other
arch to pick default without any tuning and also this will remove the
compile time option to disable the feature?
Thanks for review
With regards
Maddy
On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
quoted
Hugh Dickins [off-list ref] writes:
quoted
On Thu, 15 May 2014, Madhavan Srinivasan wrote:
quoted
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
quoted
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
Sorry for not commenting earlier - just reminded by this ping to Ingo.
I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
arch/powerpc/Kconfig suggests that Power supports base page size of
4k, 16k, 64k or 256k.
I would expect your optimal fault_around_order to depend very much on
the base page size.
It was 64k, which is what PPC64 uses on all the major distributions.
You really only get a choice of 4k and 64k with 64 bit power.
This is true. PPC64 support multiple pagesize and yes the default page
size of 64k, is taken as base pagesize for the tests.
quoted
quoted
Perhaps fault_around_size would provide a more useful default?
That seems to fit. With 4k pages and order 4, you're asking for 64k.
Maddy's result shows 64k is also reasonable for 64k pages.
Perhaps we try to generalize from two data points (a slight improvement
over doing it from 1!), eg:
/* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
unsigned int fault_around_order __read_mostly =
(16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
Rusty's bimodal answer doesn't seem the right starting point to me.
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
- when that doesn't wrap, of course!
That would at least have a better chance of being appropriate for
architectures with 8k and 16k pages (Itanium springs to mind).
Not necessarily right for them, since each architecture may have
different faulting overheads; but a better chance of being right
than blindly assuming 4k or 64k pages for everyone.
I'd be glad to see that change go into v3.15: what do you think,
Kirill, are we too late to make such a change now?
Or do you see some objection to it?
This may be right. But these are the concerns, will not this make other
arch to pick default without any tuning
Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
Did other architectures, with other page sizes, back that default?
Clearly not powerpc.
and also this will remove the
compile time option to disable the feature?
Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
for v3.16?
I'm not sure whether Rusty was arguing against that or not. I think
we are all three concerned to have a more sensible default than what's
there at present. I don't feel very strongly about your Kconfig
option: I've no objection, if it were to default to byte order 16.
Hugh
From: Andrew Morton <akpm@linux-foundation.org> Date: 2014-05-19 23:43:06
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
From: Kirill A. Shutemov <hidden> Date: 2014-05-20 00:44:38
Andrew Morton wrote:
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
I'll sign it off tomorrow after testing.
On Tuesday 20 May 2014 04:53 AM, Hugh Dickins wrote:
On Mon, 19 May 2014, Madhavan Srinivasan wrote:
quoted
On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
quoted
Hugh Dickins [off-list ref] writes:
quoted
On Thu, 15 May 2014, Madhavan Srinivasan wrote:
quoted
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
quoted
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
Sorry for not commenting earlier - just reminded by this ping to Ingo.
I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
arch/powerpc/Kconfig suggests that Power supports base page size of
4k, 16k, 64k or 256k.
I would expect your optimal fault_around_order to depend very much on
the base page size.
It was 64k, which is what PPC64 uses on all the major distributions.
You really only get a choice of 4k and 64k with 64 bit power.
This is true. PPC64 support multiple pagesize and yes the default page
size of 64k, is taken as base pagesize for the tests.
quoted
quoted
Perhaps fault_around_size would provide a more useful default?
That seems to fit. With 4k pages and order 4, you're asking for 64k.
Maddy's result shows 64k is also reasonable for 64k pages.
Perhaps we try to generalize from two data points (a slight improvement
over doing it from 1!), eg:
/* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
unsigned int fault_around_order __read_mostly =
(16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
Rusty's bimodal answer doesn't seem the right starting point to me.
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
- when that doesn't wrap, of course!
That would at least have a better chance of being appropriate for
architectures with 8k and 16k pages (Itanium springs to mind).
Not necessarily right for them, since each architecture may have
different faulting overheads; but a better chance of being right
than blindly assuming 4k or 64k pages for everyone.
I'd be glad to see that change go into v3.15: what do you think,
Kirill, are we too late to make such a change now?
Or do you see some objection to it?
quoted
This may be right. But these are the concerns, will not this make other
arch to pick default without any tuning
Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
Did other architectures, with other page sizes, back that default?
Clearly not powerpc.
Ok.
quoted
and also this will remove the
compile time option to disable the feature?
Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
for v3.16?
I'm not sure whether Rusty was arguing against that or not I think
we are all three concerned to have a more sensible default than what's
there at present. I don't feel very strongly about your Kconfig
Added it as one way to reset or disable the default value. But then I
guess we decided on having FAULT_AROUND_ORDER as a variable which is
more important than Kconfig option.
option: I've no objection, if it were to default to byte order 16.
From: Rusty Russell <hidden> Date: 2014-05-20 02:10:09
Hugh Dickins [off-list ref] writes:
On Mon, 19 May 2014, Madhavan Srinivasan wrote:
quoted
On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
quoted
Hugh Dickins [off-list ref] writes:
quoted
On Thu, 15 May 2014, Madhavan Srinivasan wrote:
quoted
Hi Ingo,
Do you have any comments for the latest version of the patchset. If
not, kindly can you pick it up as is.
With regards
Maddy
quoted
Kirill A. Shutemov with 8c6e50b029 commit introduced
vm_ops->map_pages() for mapping easy accessible pages around
fault address in hope to reduce number of minor page faults.
This patch creates infrastructure to modify the FAULT_AROUND_ORDER
value using mm/Kconfig. This will enable architecture maintainers
to decide on suitable FAULT_AROUND_ORDER value based on
performance data for that architecture. First patch also defaults
FAULT_AROUND_ORDER Kconfig element to 4. Second patch list
out the performance numbers for powerpc (platform pseries) and
initialize the fault around order variable for pseries platform of
powerpc.
Sorry for not commenting earlier - just reminded by this ping to Ingo.
I didn't study your numbers, but nowhere did I see what PAGE_SIZE you use.
arch/powerpc/Kconfig suggests that Power supports base page size of
4k, 16k, 64k or 256k.
I would expect your optimal fault_around_order to depend very much on
the base page size.
It was 64k, which is what PPC64 uses on all the major distributions.
You really only get a choice of 4k and 64k with 64 bit power.
This is true. PPC64 support multiple pagesize and yes the default page
size of 64k, is taken as base pagesize for the tests.
quoted
quoted
Perhaps fault_around_size would provide a more useful default?
That seems to fit. With 4k pages and order 4, you're asking for 64k.
Maddy's result shows 64k is also reasonable for 64k pages.
Perhaps we try to generalize from two data points (a slight improvement
over doing it from 1!), eg:
/* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
unsigned int fault_around_order __read_mostly =
(16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
Rusty's bimodal answer doesn't seem the right starting point to me.
? It's not bimodal, it's graded. I think you misread?
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
- when that doesn't wrap, of course!
That would at least have a better chance of being appropriate for
architectures with 8k and 16k pages (Itanium springs to mind).
Well, from our two data points it seems that we want to fault in
64k at a time whatever our page size. Perhaps it's clearer if the
code expresses itself that way.
Wasn't FAULT_AROUND_ORDER 4 chosen solely on the basis of x86 4k pages?
Did other architectures, with other page sizes, back that default?
Clearly not powerpc.
Yeah, BenH flagged it as "we should test this" for powerpc, which is
what Maddy then did.
quoted
and also this will remove the
compile time option to disable the feature?
Compile time option meaning your FAULT_AROUND_ORDER in mm/Kconfig
for v3.16?
I'm not sure whether Rusty was arguing against that or not. I think
we are all three concerned to have a more sensible default than what's
there at present. I don't feel very strongly about your Kconfig
option: I've no objection, if it were to default to byte order 16.
On Monday 19 May 2014 05:42 AM, Rusty Russell wrote:
quoted
Perhaps we try to generalize from two data points (a slight improvement
over doing it from 1!), eg:
/* 4 seems good for 4k-page x86, 0 seems good for 64k page ppc64, so: */
unsigned int fault_around_order __read_mostly =
(16 - PAGE_SHIFT < 0 ? 0 : 16 - PAGE_SHIFT);
Rusty's bimodal answer doesn't seem the right starting point to me.
? It's not bimodal, it's graded. I think you misread?
Yikes, worse than misread, more like I was too rude even to read: sorry!
Hugh
From: Rusty Russell <hidden> Date: 2014-05-20 07:17:11
"Kirill A. Shutemov" [off-list ref] writes:
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Cheers,
Rusty.
This isn't an appropriate header file for exporting something from core
mm - what happens if arch/mn10300 wants it?.
I guess include/linux/mm.h is the place.
From: Andrew Morton <akpm@linux-foundation.org> Date: 2014-05-20 07:32:25
On Tue, 20 May 2014 15:52:07 +0930 Rusty Russell [off-list ref] wrote:
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
We're at 3.15-rc5 and this interface should be finalised for 3.16. So
Kirrill's patch is pretty urgent and should come first.
Well. It's only a debugfs interface at this stage so we are allowed to
change it later, but it's better not to.
On Tuesday 20 May 2014 01:02 PM, Andrew Morton wrote:
On Tue, 20 May 2014 15:52:07 +0930 Rusty Russell [off-list ref] wrote:
quoted
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
We're at 3.15-rc5 and this interface should be finalised for 3.16. So
Kirrill's patch is pretty urgent and should come first.
Well. It's only a debugfs interface at this stage so we are allowed to
change it later, but it's better not to.
My patchset does not change the interface, but uses the current fault
around order variable from CONFIG_DEBUG_FS block to allow changes at
runtime, instead of having a constant and some cleanup.
Thanks for review
Regards
--Maddy
This isn't an appropriate header file for exporting something from core
mm - what happens if arch/mn10300 wants it?.
I guess include/linux/mm.h is the place.
Rusty already suggested this. My bad. Reason for adding it here was
that, I did the performance test for this platform. Will change and send
it out.
Thanks for review
Regards
Maddy
From: Kirill A. Shutemov <hidden> Date: 2014-05-20 10:27:47
Rusty Russell wrote:
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
@@ -3402,63 +3402,47 @@ void do_set_pte(struct vm_area_struct *vma, unsigned long address,update_mmu_cache(vma,address,pte);}-#define FAULT_AROUND_ORDER 4+staticunsignedlongfault_around_bytes=65536;++staticinlineunsignedlongfault_around_pages(void)+{+returnrounddown_pow_of_two(fault_around_bytes)/PAGE_SIZE;+}++staticinlineunsignedlongfault_around_mask(void)+{+return~(rounddown_pow_of_two(fault_around_bytes)-1)&PAGE_MASK;+}-#ifdef CONFIG_DEBUG_FS-staticunsignedintfault_around_order=FAULT_AROUND_ORDER;-staticintfault_around_order_get(void*data,u64*val)+#ifdef CONFIG_DEBUG_FS+staticintfault_around_bytes_get(void*data,u64*val){-*val=fault_around_order;+*val=fault_around_bytes;return0;}-staticintfault_around_order_set(void*data,u64val)+staticintfault_around_bytes_set(void*data,u64val){-BUILD_BUG_ON((1UL<<FAULT_AROUND_ORDER)>PTRS_PER_PTE);-if(1UL<<val>PTRS_PER_PTE)+if(val/PAGE_SIZE>PTRS_PER_PTE)return-EINVAL;-fault_around_order=val;+fault_around_bytes=val;return0;}-DEFINE_SIMPLE_ATTRIBUTE(fault_around_order_fops,-fault_around_order_get,fault_around_order_set,"%llu\n");+DEFINE_SIMPLE_ATTRIBUTE(fault_around_bytes_fops,+fault_around_bytes_get,fault_around_bytes_set,"%llu\n");staticint__initfault_around_debugfs(void){void*ret;-ret=debugfs_create_file("fault_around_order",0644,NULL,NULL,-&fault_around_order_fops);+ret=debugfs_create_file("fault_around_bytes",0644,NULL,NULL,+&fault_around_bytes_fops);if(!ret)-pr_warn("Failed to create fault_around_order in debugfs");+pr_warn("Failed to create fault_around_bytes in debugfs");return0;}late_initcall(fault_around_debugfs);--staticinlineunsignedlongfault_around_pages(void)-{-return1UL<<fault_around_order;-}--staticinlineunsignedlongfault_around_mask(void)-{-return~((1UL<<(PAGE_SHIFT+fault_around_order))-1);-}-#else-staticinlineunsignedlongfault_around_pages(void)-{-unsignedlongnr_pages;--nr_pages=1UL<<FAULT_AROUND_ORDER;-BUILD_BUG_ON(nr_pages>PTRS_PER_PTE);-returnnr_pages;-}--staticinlineunsignedlongfault_around_mask(void)-{-return~((1UL<<(PAGE_SHIFT+FAULT_AROUND_ORDER))-1);-}#endifstaticvoiddo_fault_around(structvm_area_struct*vma,unsignedlongaddress,
From: Andrew Morton <akpm@linux-foundation.org> Date: 2014-05-20 19:59:59
On Tue, 20 May 2014 13:27:38 +0300 (EEST) "Kirill A. Shutemov" [off-list ref] wrote:
Rusty Russell wrote:
quoted
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
This appears to be against mainline, not against Madhavan's patch. As
mentioned previously, I'd prefer it that way but confused.
quoted hunk
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
I think we should round up, not down. So if the user asks for 1kb,
they get one page.
So this becomes
return PAGE_ALIGN(fault_around_bytes) / PAGE_SIZE;
And this has me a bit stumped. It's not helpful that do_fault_around()
is undocumented. Does it fault in N/2 pages ahead and N/2 pages
behind? Or does it align the address down to the highest multiple of
fault_around_bytes? It appears to be the latter, so the location of
the faultaround window around the fault address is basically random,
depending on what address userspace happened to pick. I don't know why
we did this :(
Or something. Can we please get some code commentary over
do_fault_around() describing this design decision and explaining the
reasoning behind it?
Also, "neast" is not a word.
From: Kirill A. Shutemov <hidden> Date: 2014-05-21 13:40:42
Andrew Morton wrote:
On Tue, 20 May 2014 13:27:38 +0300 (EEST) "Kirill A. Shutemov" [off-list ref] wrote:
quoted
Rusty Russell wrote:
quoted
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
This appears to be against mainline, not against Madhavan's patch. As
mentioned previously, I'd prefer it that way but confused.
quoted
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
I think we should round up, not down. So if the user asks for 1kb,
they get one page.
So this becomes
return PAGE_ALIGN(fault_around_bytes) / PAGE_SIZE;
And this has me a bit stumped. It's not helpful that do_fault_around()
is undocumented. Does it fault in N/2 pages ahead and N/2 pages
behind? Or does it align the address down to the highest multiple of
fault_around_bytes? It appears to be the latter, so the location of
the faultaround window around the fault address is basically random,
depending on what address userspace happened to pick. I don't know why
we did this :(
When we call ->map_pages() we need to make sure that we stay within VMA
and the page table. We don't want to cross page table boundary, because
page table is what ptlock covers in split ptlock case.
I've designed the feature with fault area nominated in page order in mind
and I found it's easier to make sure we don't cross boundaries, if we
would align virtual address of fault around area to PAGE_SIZE <<
FAULT_AROUND_ORDER.
And yes fault address may be anywhere within the area. You can think about
this as a virtual page with size PAGE_SIZE << FAULT_AROUND_ORDER: no matter
what is fault address, we handle area naturally aligned to page size which
fault address belong to.
I've used rounddown_pow_of_two() in the patch to align to nearest page
order, not to page size, because that's what current do_fault_around()
expect to see. And roundup is not an option: nobody expects fault around
area to be 128k if fault_around_bytes set to 64k + 1 bytes.
If you think we need this I can rework do_fault_around() to handle
non-pow-of-two fault_around_pages(), but I don't think it's good idea to
do this for v3.15. Anyway, patch I've proposed allows change
fault_around_bytes only from DEBUG_FS and roundown should be good
enough there.
Or something. Can we please get some code commentary over
do_fault_around() describing this design decision and explaining the
reasoning behind it?
I'll do this. But if do_fault_around() rework is needed, I want to do that
first.
Also, "neast" is not a word.
:facepalm:
From: "Kirill A. Shutemov" <redacted>
Date: Wed, 21 May 2014 16:36:42 +0300
Subject: [PATCH] mm: fix typo in comment in do_fault_around()
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Andrew Morton <akpm@linux-foundation.org> Date: 2014-05-21 20:34:13
On Wed, 21 May 2014 16:40:27 +0300 (EEST) "Kirill A. Shutemov" [off-list ref] wrote:
quoted
Or something. Can we please get some code commentary over
do_fault_around() describing this design decision and explaining the
reasoning behind it?
I'll do this. But if do_fault_around() rework is needed, I want to do that
first.
This sort of thing should be at least partially driven by observation
and I don't have the data for that. My seat of the pants feel is that
after the first fault, accesses at higher addresses are more
common/probable than accesses at lower addresses. In which case we
should see improvements by centering the window at some higher address
than the fault. Much instrumentation and downstream analysis is needed
and the returns will be pretty small!
But we don't need to do all that right now. Let's get the current
implementation wrapped up for 3.15: get the interface finalized (bytes,
not pages!) and get the current design decisions appropriately
documented.
From: Kirill A. Shutemov <hidden> Date: 2014-05-23 12:29:23
Andrew Morton wrote:
On Wed, 21 May 2014 16:40:27 +0300 (EEST) "Kirill A. Shutemov" [off-list ref] wrote:
quoted
quoted
Or something. Can we please get some code commentary over
do_fault_around() describing this design decision and explaining the
reasoning behind it?
I'll do this. But if do_fault_around() rework is needed, I want to do that
first.
This sort of thing should be at least partially driven by observation
and I don't have the data for that. My seat of the pants feel is that
after the first fault, accesses at higher addresses are more
common/probable than accesses at lower addresses.
It's probably true for data, but the feature is mostly targeted to code pages
and situation is not that obvious to me with all jumps.
But we don't need to do all that right now. Let's get the current
implementation wrapped up for 3.15: get the interface finalized (bytes,
not pages!)
The patch above by thread is okay for that, right?
and get the current design decisions appropriately documented.
Here it is. Based on patch to convert order->bytes.
From: "Kirill A. Shutemov" <redacted>
Date: Fri, 23 May 2014 15:16:47 +0300
Subject: [PATCH] mm: document do_fault_around() feature
Some clarification on how faultaround works.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
On Tuesday 20 May 2014 03:57 PM, Kirill A. Shutemov wrote:
quoted hunk
Rusty Russell wrote:
quoted
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
Kindly ignore the question if not relevant. Even though we need root
access to alter the value, will we be fine with
negative value?.
Regards
Maddy
quoted hunk
- BUILD_BUG_ON((1UL << FAULT_AROUND_ORDER) > PTRS_PER_PTE);
- if (1UL << val > PTRS_PER_PTE)
+ if (val / PAGE_SIZE > PTRS_PER_PTE)
return -EINVAL;
- fault_around_order = val;
+ fault_around_bytes = val;
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fault_around_order_fops,
- fault_around_order_get, fault_around_order_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fault_around_bytes_fops,
+ fault_around_bytes_get, fault_around_bytes_set, "%llu\n");
static int __init fault_around_debugfs(void)
{
void *ret;
- ret = debugfs_create_file("fault_around_order", 0644, NULL, NULL,
- &fault_around_order_fops);
+ ret = debugfs_create_file("fault_around_bytes", 0644, NULL, NULL,
+ &fault_around_bytes_fops);
if (!ret)
- pr_warn("Failed to create fault_around_order in debugfs");
+ pr_warn("Failed to create fault_around_bytes in debugfs");
return 0;
}
late_initcall(fault_around_debugfs);
-
-static inline unsigned long fault_around_pages(void)
-{
- return 1UL << fault_around_order;
-}
-
-static inline unsigned long fault_around_mask(void)
-{
- return ~((1UL << (PAGE_SHIFT + fault_around_order)) - 1);
-}
-#else
-static inline unsigned long fault_around_pages(void)
-{
- unsigned long nr_pages;
-
- nr_pages = 1UL << FAULT_AROUND_ORDER;
- BUILD_BUG_ON(nr_pages > PTRS_PER_PTE);
- return nr_pages;
-}
-
-static inline unsigned long fault_around_mask(void)
-{
- return ~((1UL << (PAGE_SHIFT + FAULT_AROUND_ORDER)) - 1);
-}
#endif
static void do_fault_around(struct vm_area_struct *vma, unsigned long address,
@@ -3515,7 +3499,7 @@ static int do_read_fault(struct mm_struct *mm, struct vm_area_struct *vma, * if page by the offset is not ready to be mapped (cold cache or * something). */- if (vma->vm_ops->map_pages) {+ if (vma->vm_ops->map_pages && fault_around_pages() > 1) { pte = pte_offset_map_lock(mm, pmd, address, &ptl); do_fault_around(vma, address, pte, pgoff, flags); if (!pte_same(*pte, orig_pte))
From: Kirill A. Shutemov <hidden> Date: 2014-05-27 10:22:12
Madhavan Srinivasan wrote:
On Tuesday 20 May 2014 03:57 PM, Kirill A. Shutemov wrote:
quoted
Rusty Russell wrote:
quoted
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)
On Tuesday 27 May 2014 03:51 PM, Kirill A. Shutemov wrote:
Madhavan Srinivasan wrote:
quoted
On Tuesday 20 May 2014 03:57 PM, Kirill A. Shutemov wrote:
quoted
Rusty Russell wrote:
quoted
"Kirill A. Shutemov" [off-list ref] writes:
quoted
Andrew Morton wrote:
quoted
On Mon, 19 May 2014 16:23:07 -0700 (PDT) Hugh Dickins [off-list ref] wrote:
quoted
Shouldn't FAULT_AROUND_ORDER and fault_around_order be changed to be
the order of the fault-around size in bytes, and fault_around_pages()
use 1UL << (fault_around_order - PAGE_SHIFT)
Yes. And shame on me for missing it (this time!) at review.
There's still time to fix this. Patches, please.
Here it is. Made at 3.30 AM, build tested only.
Prefer on top of Maddy's patch which makes it always a variable, rather
than CONFIG_DEBUG_FS. It's got enough hair as it is.
Something like this?
From: "Kirill A. Shutemov" <redacted>
Date: Tue, 20 May 2014 13:02:03 +0300
Subject: [PATCH] mm: nominate faultaround area in bytes rather then page order
There are evidences that faultaround feature is less relevant on
architectures with page size bigger then 4k. Which makes sense since
page fault overhead per byte of mapped area should be less there.
Let's rework the feature to specify faultaround area in bytes instead of
page order. It's 64 kilobytes for now.
The patch effectively disables faultaround on architectures with
page size >= 64k (like ppc64).
It's possible that some other size of faultaround area is relevant for a
platform. We can expose `fault_around_bytes' variable to arch-specific
code once such platforms will be found.
Signed-off-by: Kirill A. Shutemov <redacted>
---
mm/memory.c | 62 +++++++++++++++++++++++--------------------------------------
1 file changed, 23 insertions(+), 39 deletions(-)