As 'delta' is an unsigned long, 'end' (vma->vm_end + delta) cannot
be less than 'vma->vm_end'. Checking for availability of virtual
address range at the end of the VMA for the incremental size is
also reduntant at this point. Hence drop them both.
Signed-off-by: Anshuman Khandual <[email protected]>
---
The following test program achieves fatser execution time with
this change.
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <errno.h>
#include <sys/mman.h>
#include <sys/time.h>
#define ALLOC_SIZE 0x10000UL
#define MAX_COUNT 1024 * 1024
int main(int argc, char *argv[])
{
unsigned long count;
char *ptr;
ptr = mmap(NULL, ALLOC_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE| MAP_ANONYMOUS, -1, 0);
if (ptr == MAP_FAILED) {
perror("map() failed");
return -1;
}
memset(ptr, 0, ALLOC_SIZE);
for (count = 1; count <= MAX_COUNT; count++) {
ptr = (char *) mremap(ptr, ALLOC_SIZE * count, ALLOC_SIZE * (count + 1), 1);
if (ptr == MAP_FAILED) {
perror("mremap() failed");
printf("At %lu size", ALLOC_SIZE * (count + 1));
return -1;
}
/*
memset(ptr, 0, ALLOC_SIZE * (count + 1));
*/
}
for (count = MAX_COUNT; count > 1; count--) {
ptr = (char *) mremap(ptr, ALLOC_SIZE * count, ALLOC_SIZE * (count - 1), 1);
if (ptr == MAP_FAILED) {
perror("mremap() failed");
printf("At %lu size", ALLOC_SIZE * (count - 1));
return -1;
}
/*
memset(ptr, 0, ALLOC_SIZE * (count - 1));
*/
}
return 0;
}
mm/mremap.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/mm/mremap.c b/mm/mremap.c
index cd8a1b1..b937c28 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -487,12 +487,9 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
static int vma_expandable(struct vm_area_struct *vma, unsigned long delta)
{
unsigned long end = vma->vm_end + delta;
- if (end < vma->vm_end) /* overflow */
- return 0;
- if (vma->vm_next && vma->vm_next->vm_start < end) /* intersection */
- return 0;
- if (get_unmapped_area(NULL, vma->vm_start, end - vma->vm_start,
- 0, MAP_FIXED) & ~PAGE_MASK)
+
+ /* Intersection with next VMA */
+ if (vma->vm_next && vma->vm_next->vm_start < end)
return 0;
return 1;
}
--
1.8.5.2
On Mon 10-07-17 16:40:59, Anshuman Khandual wrote:
> As 'delta' is an unsigned long, 'end' (vma->vm_end + delta) cannot
> be less than 'vma->vm_end'.
This just doesn't make any sense. This is exactly what the overflow
check is for. Maybe vm_end + delta can never overflow because of
(old_len == vma->vm_end - addr) and guarantee old_len < new_len
in mremap but I haven't checked that too deeply.
> Checking for availability of virtual
> address range at the end of the VMA for the incremental size is
> also reduntant at this point. Hence drop them both.
OK, this seems to be the case due the above (comment says "old_len
exactly to the end of the area..").
But I am wondering what led you to the patch because you do not say so
here. This is hardly something that would save many cycles in a
relatively cold path.
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
>
> The following test program achieves fatser execution time with
> this change.
>
> #include <stdio.h>
> #include <string.h>
> #include <unistd.h>
> #include <errno.h>
> #include <sys/mman.h>
> #include <sys/time.h>
>
> #define ALLOC_SIZE 0x10000UL
> #define MAX_COUNT 1024 * 1024
>
> int main(int argc, char *argv[])
> {
> unsigned long count;
> char *ptr;
>
> ptr = mmap(NULL, ALLOC_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE| MAP_ANONYMOUS, -1, 0);
> if (ptr == MAP_FAILED) {
> perror("map() failed");
> return -1;
> }
> memset(ptr, 0, ALLOC_SIZE);
>
> for (count = 1; count <= MAX_COUNT; count++) {
> ptr = (char *) mremap(ptr, ALLOC_SIZE * count, ALLOC_SIZE * (count + 1), 1);
> if (ptr == MAP_FAILED) {
> perror("mremap() failed");
> printf("At %lu size", ALLOC_SIZE * (count + 1));
> return -1;
> }
> /*
> memset(ptr, 0, ALLOC_SIZE * (count + 1));
> */
> }
>
>
> for (count = MAX_COUNT; count > 1; count--) {
> ptr = (char *) mremap(ptr, ALLOC_SIZE * count, ALLOC_SIZE * (count - 1), 1);
> if (ptr == MAP_FAILED) {
> perror("mremap() failed");
> printf("At %lu size", ALLOC_SIZE * (count - 1));
> return -1;
> }
> /*
> memset(ptr, 0, ALLOC_SIZE * (count - 1));
> */
> }
> return 0;
> }
>
>
> mm/mremap.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index cd8a1b1..b937c28 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -487,12 +487,9 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> static int vma_expandable(struct vm_area_struct *vma, unsigned long delta)
> {
> unsigned long end = vma->vm_end + delta;
> - if (end < vma->vm_end) /* overflow */
> - return 0;
> - if (vma->vm_next && vma->vm_next->vm_start < end) /* intersection */
> - return 0;
> - if (get_unmapped_area(NULL, vma->vm_start, end - vma->vm_start,
> - 0, MAP_FIXED) & ~PAGE_MASK)
> +
> + /* Intersection with next VMA */
> + if (vma->vm_next && vma->vm_next->vm_start < end)
> return 0;
> return 1;
> }
> --
> 1.8.5.2
--
Michal Hocko
SUSE Labs
On 07/10/2017 07:19 PM, Michal Hocko wrote:
> On Mon 10-07-17 16:40:59, Anshuman Khandual wrote:
>> As 'delta' is an unsigned long, 'end' (vma->vm_end + delta) cannot
>> be less than 'vma->vm_end'.
>
> This just doesn't make any sense. This is exactly what the overflow
> check is for. Maybe vm_end + delta can never overflow because of
> (old_len == vma->vm_end - addr) and guarantee old_len < new_len
> in mremap but I haven't checked that too deeply.
Irrespective of that, just looking at the variables inside this
particular function where delta is an 'unsigned long', 'end' cannot
be less than vma->vm_end. Is not that true ?
>
>> Checking for availability of virtual
>> address range at the end of the VMA for the incremental size is
>> also reduntant at this point. Hence drop them both.
>
> OK, this seems to be the case due the above (comment says "old_len
> exactly to the end of the area..").
yeah but is the check necessary ?
>
> But I am wondering what led you to the patch because you do not say so
As can be seen in the test program, was trying to measure the speed
of VMA expansion and contraction inside an address space and then
figured out that dropping this check improves the speed prima facie.
> here. This is hardly something that would save many cycles in a
> relatively cold path.
Though I have not done any detailed instruction level measurement,
there is a reduction in real and system amount of time to execute
the test with and without the patch.
Without the patch
real 0m2.100s
user 0m0.162s
sys 0m1.937s
With this patch
real 0m0.928s
user 0m0.161s
sys 0m0.756s
On 07/10/2017 07:19 PM, Michal Hocko wrote:
> On Mon 10-07-17 16:40:59, Anshuman Khandual wrote:
>> As 'delta' is an unsigned long, 'end' (vma->vm_end + delta) cannot
>> be less than 'vma->vm_end'.
>
> This just doesn't make any sense. This is exactly what the overflow
> check is for. Maybe vm_end + delta can never overflow because of
> (old_len == vma->vm_end - addr) and guarantee old_len < new_len
> in mremap but I haven't checked that too deeply.
Irrespective of that, just looking at the variables inside this
particular function where delta is an 'unsigned long', 'end' cannot
be less than vma->vm_end. Is not that true ?
>
>> Checking for availability of virtual
>> address range at the end of the VMA for the incremental size is
>> also reduntant at this point. Hence drop them both.
>
> OK, this seems to be the case due the above (comment says "old_len
> exactly to the end of the area..").
yeah but is the check necessary ?
>
> But I am wondering what led you to the patch because you do not say so
As can be seen in the test program, was trying to measure the speed
of VMA expansion and contraction inside an address space and then
figured out that dropping this check improves the speed prima facie.
> here. This is hardly something that would save many cycles in a
> relatively cold path.
Though I have not done any detailed instruction level measurement,
there is a reduction in real and system amount of time to execute
the test with and without the patch.
Without the patch
real 0m2.100s
user 0m0.162s
sys 0m1.937s
With this patch
real 0m0.928s
user 0m0.161s
sys 0m0.756s
On Tue 11-07-17 09:58:42, Anshuman Khandual wrote:
> On 07/10/2017 07:19 PM, Michal Hocko wrote:
> > On Mon 10-07-17 16:40:59, Anshuman Khandual wrote:
> >> As 'delta' is an unsigned long, 'end' (vma->vm_end + delta) cannot
> >> be less than 'vma->vm_end'.
> >
> > This just doesn't make any sense. This is exactly what the overflow
> > check is for. Maybe vm_end + delta can never overflow because of
> > (old_len == vma->vm_end - addr) and guarantee old_len < new_len
> > in mremap but I haven't checked that too deeply.
>
> Irrespective of that, just looking at the variables inside this
> particular function where delta is an 'unsigned long', 'end' cannot
> be less than vma->vm_end. Is not that true ?
no. What happens when end is too large?
[...]
> > here. This is hardly something that would save many cycles in a
> > relatively cold path.
>
> Though I have not done any detailed instruction level measurement,
> there is a reduction in real and system amount of time to execute
> the test with and without the patch.
>
> Without the patch
>
> real 0m2.100s
> user 0m0.162s
> sys 0m1.937s
>
> With this patch
>
> real 0m0.928s
> user 0m0.161s
> sys 0m0.756s
Are you telling me that two if conditions cause more than a second
difference? That sounds suspicious.
--
Michal Hocko
SUSE Labs
On 07/11/2017 08:03 AM, Michal Hocko wrote:
> On Tue 11-07-17 09:58:42, Anshuman Khandual wrote:
>>> here. This is hardly something that would save many cycles in a
>>> relatively cold path.
>>
>> Though I have not done any detailed instruction level measurement,
>> there is a reduction in real and system amount of time to execute
>> the test with and without the patch.
>>
>> Without the patch
>>
>> real 0m2.100s
>> user 0m0.162s
>> sys 0m1.937s
>>
>> With this patch
>>
>> real 0m0.928s
>> user 0m0.161s
>> sys 0m0.756s
>
> Are you telling me that two if conditions cause more than a second
> difference? That sounds suspicious.
It's removing also a call to get_unmapped_area(), AFAICS. That means a
vma search?
On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
> On 07/11/2017 08:03 AM, Michal Hocko wrote:
> > On Tue 11-07-17 09:58:42, Anshuman Khandual wrote:
> >>> here. This is hardly something that would save many cycles in a
> >>> relatively cold path.
> >>
> >> Though I have not done any detailed instruction level measurement,
> >> there is a reduction in real and system amount of time to execute
> >> the test with and without the patch.
> >>
> >> Without the patch
> >>
> >> real 0m2.100s
> >> user 0m0.162s
> >> sys 0m1.937s
> >>
> >> With this patch
> >>
> >> real 0m0.928s
> >> user 0m0.161s
> >> sys 0m0.756s
> >
> > Are you telling me that two if conditions cause more than a second
> > difference? That sounds suspicious.
>
> It's removing also a call to get_unmapped_area(), AFAICS. That means a
> vma search?
Ohh, right. I have somehow missed that. Is this removal intentional? The
changelog is silent about it.
--
Michal Hocko
SUSE Labs
On 07/11/2017 08:50 AM, Michal Hocko wrote:
> On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
>> On 07/11/2017 08:03 AM, Michal Hocko wrote:
>>>
>>> Are you telling me that two if conditions cause more than a second
>>> difference? That sounds suspicious.
>>
>> It's removing also a call to get_unmapped_area(), AFAICS. That means a
>> vma search?
>
> Ohh, right. I have somehow missed that. Is this removal intentional?
I think it is: "Checking for availability of virtual address range at
the end of the VMA for the incremental size is also reduntant at this
point."
> The
> changelog is silent about it.
It doesn't explain why it's redundant, indeed. Unfortunately, the commit
f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
also doesn't explain why it's needed.
On Tue 11-07-17 08:56:04, Vlastimil Babka wrote:
> On 07/11/2017 08:50 AM, Michal Hocko wrote:
> > On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
> >> On 07/11/2017 08:03 AM, Michal Hocko wrote:
> >>>
> >>> Are you telling me that two if conditions cause more than a second
> >>> difference? That sounds suspicious.
> >>
> >> It's removing also a call to get_unmapped_area(), AFAICS. That means a
> >> vma search?
> >
> > Ohh, right. I have somehow missed that. Is this removal intentional?
>
> I think it is: "Checking for availability of virtual address range at
> the end of the VMA for the incremental size is also reduntant at this
> point."
I though this referred to this check
if (vma->vm_next && vma->vm_next->vm_start < end)
becuase get_unampped_area with MAP_FIXED doesn't really check
anything. It will simply return the given address. Btw. this also rules
out find_vma.
> > The
> > changelog is silent about it.
>
> It doesn't explain why it's redundant, indeed. Unfortunately, the commit
> f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
> also doesn't explain why it's needed.
Because it doesn't do anything AFAICS.
--
Michal Hocko
SUSE Labs
On Tue 11-07-17 09:16:12, Michal Hocko wrote:
> On Tue 11-07-17 08:56:04, Vlastimil Babka wrote:
[...]
> > It doesn't explain why it's redundant, indeed. Unfortunately, the commit
> > f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
> > also doesn't explain why it's needed.
>
> Because it doesn't do anything AFAICS.
Well, it does actually. I have missed security_mmap_addr hook.
--
Michal Hocko
SUSE Labs
On 07/11/2017 11:56 AM, Vlastimil Babka wrote:
> On 07/11/2017 08:03 AM, Michal Hocko wrote:
>> On Tue 11-07-17 09:58:42, Anshuman Khandual wrote:
>>>> here. This is hardly something that would save many cycles in a
>>>> relatively cold path.
>>> Though I have not done any detailed instruction level measurement,
>>> there is a reduction in real and system amount of time to execute
>>> the test with and without the patch.
>>>
>>> Without the patch
>>>
>>> real 0m2.100s
>>> user 0m0.162s
>>> sys 0m1.937s
>>>
>>> With this patch
>>>
>>> real 0m0.928s
>>> user 0m0.161s
>>> sys 0m0.756s
>> Are you telling me that two if conditions cause more than a second
>> difference? That sounds suspicious.
> It's removing also a call to get_unmapped_area(), AFAICS. That means a
> vma search?
I believe removing this function is responsible for the
increase in speed of the test execution.
On 07/11/2017 12:20 PM, Michal Hocko wrote:
> On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
>> On 07/11/2017 08:03 AM, Michal Hocko wrote:
>>> On Tue 11-07-17 09:58:42, Anshuman Khandual wrote:
>>>>> here. This is hardly something that would save many cycles in a
>>>>> relatively cold path.
>>>> Though I have not done any detailed instruction level measurement,
>>>> there is a reduction in real and system amount of time to execute
>>>> the test with and without the patch.
>>>>
>>>> Without the patch
>>>>
>>>> real 0m2.100s
>>>> user 0m0.162s
>>>> sys 0m1.937s
>>>>
>>>> With this patch
>>>>
>>>> real 0m0.928s
>>>> user 0m0.161s
>>>> sys 0m0.756s
>>> Are you telling me that two if conditions cause more than a second
>>> difference? That sounds suspicious.
>> It's removing also a call to get_unmapped_area(), AFAICS. That means a
>> vma search?
> Ohh, right. I have somehow missed that. Is this removal intentional? The
> changelog is silent about it.
Yeah it was.
On 07/11/2017 12:26 PM, Vlastimil Babka wrote:
> On 07/11/2017 08:50 AM, Michal Hocko wrote:
>> On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
>>> On 07/11/2017 08:03 AM, Michal Hocko wrote:
>>>>
>>>> Are you telling me that two if conditions cause more than a second
>>>> difference? That sounds suspicious.
>>>
>>> It's removing also a call to get_unmapped_area(), AFAICS. That means a
>>> vma search?
>>
>> Ohh, right. I have somehow missed that. Is this removal intentional?
>
> I think it is: "Checking for availability of virtual address range at
> the end of the VMA for the incremental size is also reduntant at this
> point."
>
>> The
>> changelog is silent about it.
>
> It doesn't explain why it's redundant, indeed. Unfortunately, the commit
> f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
> also doesn't explain why it's needed.
Its redundant because there are calls to get_unmapped_area() down the
line in the function whose failure will anyway fail the expansion of
the VMA.
On 07/11/2017 12:46 PM, Michal Hocko wrote:
> On Tue 11-07-17 08:56:04, Vlastimil Babka wrote:
>> On 07/11/2017 08:50 AM, Michal Hocko wrote:
>>> On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
>>>> On 07/11/2017 08:03 AM, Michal Hocko wrote:
>>>>>
>>>>> Are you telling me that two if conditions cause more than a second
>>>>> difference? That sounds suspicious.
>>>>
>>>> It's removing also a call to get_unmapped_area(), AFAICS. That means a
>>>> vma search?
>>>
>>> Ohh, right. I have somehow missed that. Is this removal intentional?
>>
>> I think it is: "Checking for availability of virtual address range at
>> the end of the VMA for the incremental size is also reduntant at this
>> point."
>
> I though this referred to this check
> if (vma->vm_next && vma->vm_next->vm_start < end)
No, that check is still there in the code.
On 07/11/2017 12:52 PM, Michal Hocko wrote:
> On Tue 11-07-17 09:16:12, Michal Hocko wrote:
>> On Tue 11-07-17 08:56:04, Vlastimil Babka wrote:
> [...]
>>> It doesn't explain why it's redundant, indeed. Unfortunately, the commit
>>> f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
>>> also doesn't explain why it's needed.
>>
>> Because it doesn't do anything AFAICS.
>
> Well, it does actually. I have missed security_mmap_addr hook.
But we any way call get_unmapped_area() down the line in the function,
it should be covered then. Does the proposed change look good and be
considered, or any changes required or can be dropped ?
On Tue 11-07-17 16:38:46, Anshuman Khandual wrote:
> On 07/11/2017 12:26 PM, Vlastimil Babka wrote:
> > On 07/11/2017 08:50 AM, Michal Hocko wrote:
> >> On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
> >>> On 07/11/2017 08:03 AM, Michal Hocko wrote:
> >>>>
> >>>> Are you telling me that two if conditions cause more than a second
> >>>> difference? That sounds suspicious.
> >>>
> >>> It's removing also a call to get_unmapped_area(), AFAICS. That means a
> >>> vma search?
> >>
> >> Ohh, right. I have somehow missed that. Is this removal intentional?
> >
> > I think it is: "Checking for availability of virtual address range at
> > the end of the VMA for the incremental size is also reduntant at this
> > point."
> >
> >> The
> >> changelog is silent about it.
> >
> > It doesn't explain why it's redundant, indeed. Unfortunately, the commit
> > f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
> > also doesn't explain why it's needed.
>
> Its redundant because there are calls to get_unmapped_area() down the
> line in the function whose failure will anyway fail the expansion of
> the VMA.
mremap code is quite complex and I am not sure you are right here.
Anyway, please make sure you document why you believe those checks are
not needed when resubmitting your patch.
--
Michal Hocko
SUSE Labs
On 07/11/2017 04:52 PM, Michal Hocko wrote:
> On Tue 11-07-17 16:38:46, Anshuman Khandual wrote:
>> On 07/11/2017 12:26 PM, Vlastimil Babka wrote:
>>> On 07/11/2017 08:50 AM, Michal Hocko wrote:
>>>> On Tue 11-07-17 08:26:40, Vlastimil Babka wrote:
>>>>> On 07/11/2017 08:03 AM, Michal Hocko wrote:
>>>>>> Are you telling me that two if conditions cause more than a second
>>>>>> difference? That sounds suspicious.
>>>>> It's removing also a call to get_unmapped_area(), AFAICS. That means a
>>>>> vma search?
>>>> Ohh, right. I have somehow missed that. Is this removal intentional?
>>> I think it is: "Checking for availability of virtual address range at
>>> the end of the VMA for the incremental size is also reduntant at this
>>> point."
>>>
>>>> The
>>>> changelog is silent about it.
>>> It doesn't explain why it's redundant, indeed. Unfortunately, the commit
>>> f106af4e90ea ("fix checks for expand-in-place mremap") which added this,
>>> also doesn't explain why it's needed.
>> Its redundant because there are calls to get_unmapped_area() down the
>> line in the function whose failure will anyway fail the expansion of
>> the VMA.
> mremap code is quite complex and I am not sure you are right here.
> Anyway, please make sure you document why you believe those checks are
> not needed when resubmitting your patch.
vma_adjust() expands/contracts the given VMA and adjusts everything
around it like anon vma, rb tree, statistics etc but it never checks
whether the requested expansion/contraction is OK from arch point of
view by calling get_unmapped_area() some where. IIUC now the function
vma_expandable() does that check for it beforehand. Hence I think its
required. My tests happened to pass may be because of coincidence that
every thing (all the VA space) was up for grabs at that point of time.
So, will drop this patch.