[RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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 <khandual@linux.vnet.ibm.com>
---

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

[RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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 <khandual@linux.vnet.ibm.com>
---

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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 <khandual@linux.vnet.ibm.com>
> ---
> 
> 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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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 <khandual@linux.vnet.ibm.com>
> ---
> 
> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Vlastimil Babka]

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?

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Vlastimil Babka]

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Vlastimil Babka]

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.

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Vlastimil Babka]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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 ?

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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 ?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Michal Hocko]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

Re: [RFC] mm/mremap: Remove redundant checks inside vma_expandable()

[Anshuman Khandual]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>