[PATCH] mm: Fix compile warning of mmotm-2012-09-06-16-46

[PATCH] mm: Fix compile warning of mmotm-2012-09-06-16-46

[Minchan Kim]

When I compiled today, I met following warning.
Correct it.

mm/memory.c: In function a??copy_page_rangea??:
include/linux/mmu_notifier.h:235:38: warning: a??mmun_enda?? may be used uninitialized in this function [-Wuninitialized]
mm/memory.c:1043:16: note: a??mmun_enda?? was declared here
include/linux/mmu_notifier.h:235:38: warning: a??mmun_starta?? may be used uninitialized in this function [-Wuninitialized]
mm/memory.c:1042:16: note: a??mmun_starta?? was declared here
  LD      mm/built-in.o

Cc: Sagi Grimberg <sagig@mellanox.com>
Cc: Haggai Eran <haggaie@mellanox.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 mm/memory.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 10e9b38..d000449 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1039,8 +1039,8 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	unsigned long next;
 	unsigned long addr = vma->vm_start;
 	unsigned long end = vma->vm_end;
-	unsigned long mmun_start;	/* For mmu_notifiers */
-	unsigned long mmun_end;		/* For mmu_notifiers */
+	unsigned long uninitialized_var(mmun_start);	/* For mmu_notifiers */
+	unsigned long uninitialized_var(mmun_end);	/* For mmu_notifiers */
 	int ret;
 
 	/*
-- 
1.7.9.5

--
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: [PATCH] mm: Fix compile warning of mmotm-2012-09-06-16-46

[Andrew Morton]

On Fri,  7 Sep 2012 09:57:10 +0900
Minchan Kim <minchan@kernel.org> wrote:

> When I compiled today, I met following warning.
> Correct it.
> 
> mm/memory.c: In function ___copy_page_range___:
> include/linux/mmu_notifier.h:235:38: warning: ___mmun_end___ may be used uninitialized in this function [-Wuninitialized]
> mm/memory.c:1043:16: note: ___mmun_end___ was declared here
> include/linux/mmu_notifier.h:235:38: warning: ___mmun_start___ may be used uninitialized in this function [-Wuninitialized]
> mm/memory.c:1042:16: note: ___mmun_start___ was declared here
>   LD      mm/built-in.o
> 
> Cc: Sagi Grimberg <sagig@mellanox.com>
> Cc: Haggai Eran <haggaie@mellanox.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  mm/memory.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 10e9b38..d000449 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1039,8 +1039,8 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  	unsigned long next;
>  	unsigned long addr = vma->vm_start;
>  	unsigned long end = vma->vm_end;
> -	unsigned long mmun_start;	/* For mmu_notifiers */
> -	unsigned long mmun_end;		/* For mmu_notifiers */
> +	unsigned long uninitialized_var(mmun_start);	/* For mmu_notifiers */
> +	unsigned long uninitialized_var(mmun_end);	/* For mmu_notifiers */
>  	int ret;
>  

Well yes, but a) uninitialized_var is a bit ugly and has some potential
to hide real bugs and b) it's not completely obvious that
is_cow_mapping() is stable across those two calls.

I think a better approach is this:

--- a/mm/memory.c~mm-move-all-mmu-notifier-invocations-to-be-done-outside-the-pt-lock-fix-fix
+++ a/mm/memory.c
@@ -1041,6 +1041,7 @@ int copy_page_range(struct mm_struct *ds
 	unsigned long end = vma->vm_end;
 	unsigned long mmun_start;	/* For mmu_notifiers */
 	unsigned long mmun_end;		/* For mmu_notifiers */
+	bool is_cow;
 	int ret;
 
 	/*
@@ -1074,7 +1075,8 @@ int copy_page_range(struct mm_struct *ds
 	 * parent mm. And a permission downgrade will only happen if
 	 * is_cow_mapping() returns true.
 	 */
-	if (is_cow_mapping(vma->vm_flags)) {
+	is_cow = is_cow_mapping(vma->vm_flags);
+	if (is_cow) {
 		mmun_start = addr;
 		mmun_end   = end;
 		mmu_notifier_invalidate_range_start(src_mm, mmun_start,
@@ -1095,7 +1097,7 @@ int copy_page_range(struct mm_struct *ds
 		}
 	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
 
-	if (is_cow_mapping(vma->vm_flags))
+	if (is_cow)
 		mmu_notifier_invalidate_range_end(src_mm, mmun_start, mmun_end);
 	return ret;
 }

Unfortunately, my (old) versions of gcc are so stupid that they *still*
generate the warning even when the code is as obviously correct as this :(

Can you please test it with your compiler?

We should use this patch anyway - it makes memory.o text 13 bytes smaller.

is_cow_mapping() should return bool.

--
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: [PATCH] mm: Fix compile warning of mmotm-2012-09-06-16-46

[Haggai Eran]

On Sep 7 2012 23:06, Andrew Morton wrote:
> On Fri,  7 Sep 2012 09:57:10 +0900
> Minchan Kim <minchan@kernel.org> wrote:
> 
>> When I compiled today, I met following warning.
>> Correct it.
>>
>> mm/memory.c: In function ___copy_page_range___:
>> include/linux/mmu_notifier.h:235:38: warning: ___mmun_end___ may be used uninitialized in this function [-Wuninitialized]
>> mm/memory.c:1043:16: note: ___mmun_end___ was declared here
>> include/linux/mmu_notifier.h:235:38: warning: ___mmun_start___ may be used uninitialized in this function [-Wuninitialized]
>> mm/memory.c:1042:16: note: ___mmun_start___ was declared here
>>   LD      mm/built-in.o
>>
>> Cc: Sagi Grimberg <sagig@mellanox.com>
>> Cc: Haggai Eran <haggaie@mellanox.com>
>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>> ---
>>  mm/memory.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 10e9b38..d000449 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1039,8 +1039,8 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>>  	unsigned long next;
>>  	unsigned long addr = vma->vm_start;
>>  	unsigned long end = vma->vm_end;
>> -	unsigned long mmun_start;	/* For mmu_notifiers */
>> -	unsigned long mmun_end;		/* For mmu_notifiers */
>> +	unsigned long uninitialized_var(mmun_start);	/* For mmu_notifiers */
>> +	unsigned long uninitialized_var(mmun_end);	/* For mmu_notifiers */
>>  	int ret;
>>  
> 
> Well yes, but a) uninitialized_var is a bit ugly and has some potential
> to hide real bugs and b) it's not completely obvious that
> is_cow_mapping() is stable across those two calls.
I thought that code that changed the vm_flags would need mmap_sem locked
exclusively, and copy_page_range is also called with the mmap_sem
locked, so that would prevent is_cow_mapping() from changing, wouldn't it?

> 
> I think a better approach is this:
> 
> --- a/mm/memory.c~mm-move-all-mmu-notifier-invocations-to-be-done-outside-the-pt-lock-fix-fix
> +++ a/mm/memory.c
> @@ -1041,6 +1041,7 @@ int copy_page_range(struct mm_struct *ds
>  	unsigned long end = vma->vm_end;
>  	unsigned long mmun_start;	/* For mmu_notifiers */
>  	unsigned long mmun_end;		/* For mmu_notifiers */
> +	bool is_cow;
>  	int ret;
>  
>  	/*
> @@ -1074,7 +1075,8 @@ int copy_page_range(struct mm_struct *ds
>  	 * parent mm. And a permission downgrade will only happen if
>  	 * is_cow_mapping() returns true.
>  	 */
> -	if (is_cow_mapping(vma->vm_flags)) {
> +	is_cow = is_cow_mapping(vma->vm_flags);
> +	if (is_cow) {
>  		mmun_start = addr;
>  		mmun_end   = end;
>  		mmu_notifier_invalidate_range_start(src_mm, mmun_start,
> @@ -1095,7 +1097,7 @@ int copy_page_range(struct mm_struct *ds
>  		}
>  	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
>  
> -	if (is_cow_mapping(vma->vm_flags))
> +	if (is_cow)
>  		mmu_notifier_invalidate_range_end(src_mm, mmun_start, mmun_end);
>  	return ret;
>  }
> 
> Unfortunately, my (old) versions of gcc are so stupid that they *still*
> generate the warning even when the code is as obviously correct as this :(
> 
> Can you please test it with your compiler?
With GCC 4.4.4 I still get these warnings, even with the is_cow
variable. Sorry I haven't noticed that before. Perhaps I can move the
initialization of mmun_start/end before the if.

By the way, with GCC 4.4.4 I don't get the warning in do_wp_page.

Regards,
Haggai

--
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: [PATCH] mm: Fix compile warning of mmotm-2012-09-06-16-46

[Minchan Kim]

Hi Andrew,

On Fri, Sep 07, 2012 at 01:06:05PM -0700, Andrew Morton wrote:
> On Fri,  7 Sep 2012 09:57:10 +0900
> Minchan Kim <minchan@kernel.org> wrote:
> 
> > When I compiled today, I met following warning.
> > Correct it.
> > 
> > mm/memory.c: In function ___copy_page_range___:
> > include/linux/mmu_notifier.h:235:38: warning: ___mmun_end___ may be used uninitialized in this function [-Wuninitialized]
> > mm/memory.c:1043:16: note: ___mmun_end___ was declared here
> > include/linux/mmu_notifier.h:235:38: warning: ___mmun_start___ may be used uninitialized in this function [-Wuninitialized]
> > mm/memory.c:1042:16: note: ___mmun_start___ was declared here
> >   LD      mm/built-in.o
> > 
> > Cc: Sagi Grimberg <sagig@mellanox.com>
> > Cc: Haggai Eran <haggaie@mellanox.com>
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/memory.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 10e9b38..d000449 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1039,8 +1039,8 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> >  	unsigned long next;
> >  	unsigned long addr = vma->vm_start;
> >  	unsigned long end = vma->vm_end;
> > -	unsigned long mmun_start;	/* For mmu_notifiers */
> > -	unsigned long mmun_end;		/* For mmu_notifiers */
> > +	unsigned long uninitialized_var(mmun_start);	/* For mmu_notifiers */
> > +	unsigned long uninitialized_var(mmun_end);	/* For mmu_notifiers */
> >  	int ret;
> >  
> 
> Well yes, but a) uninitialized_var is a bit ugly and has some potential
> to hide real bugs and b) it's not completely obvious that
> is_cow_mapping() is stable across those two calls.
> 
> I think a better approach is this:
> 
> --- a/mm/memory.c~mm-move-all-mmu-notifier-invocations-to-be-done-outside-the-pt-lock-fix-fix
> +++ a/mm/memory.c
> @@ -1041,6 +1041,7 @@ int copy_page_range(struct mm_struct *ds
>  	unsigned long end = vma->vm_end;
>  	unsigned long mmun_start;	/* For mmu_notifiers */
>  	unsigned long mmun_end;		/* For mmu_notifiers */
> +	bool is_cow;
>  	int ret;
>  
>  	/*
> @@ -1074,7 +1075,8 @@ int copy_page_range(struct mm_struct *ds
>  	 * parent mm. And a permission downgrade will only happen if
>  	 * is_cow_mapping() returns true.
>  	 */
> -	if (is_cow_mapping(vma->vm_flags)) {
> +	is_cow = is_cow_mapping(vma->vm_flags);
> +	if (is_cow) {
>  		mmun_start = addr;
>  		mmun_end   = end;
>  		mmu_notifier_invalidate_range_start(src_mm, mmun_start,
> @@ -1095,7 +1097,7 @@ int copy_page_range(struct mm_struct *ds
>  		}
>  	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
>  
> -	if (is_cow_mapping(vma->vm_flags))
> +	if (is_cow)
>  		mmu_notifier_invalidate_range_end(src_mm, mmun_start, mmun_end);
>  	return ret;
>  }
> 
> Unfortunately, my (old) versions of gcc are so stupid that they *still*
> generate the warning even when the code is as obviously correct as this :(
> 
> Can you please test it with your compiler?

My compiler is stupidl, too. It still emit the samw warning when I apply
your patch. In addition, I can't see any benefit of text size.

barrios@bbox:~/linux-mmotm$ size mm/memory.o mm/memory.o.andrew 
   text    data     bss     dec     hex filename
  29147      54      40   29241    7239 mm/memory.o
  29147      54      40   29241    7239 mm/memory.o.andrew


-- 
Kind regards,
Minchan Kim

--
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: [PATCH] mm: Fix compile warning of mmotm-2012-09-06-16-46

[Minchan Kim]

On Mon, Sep 10, 2012 at 10:14:07AM +0900, Minchan Kim wrote:
> Hi Andrew,
> 
> On Fri, Sep 07, 2012 at 01:06:05PM -0700, Andrew Morton wrote:
> > On Fri,  7 Sep 2012 09:57:10 +0900
> > Minchan Kim <minchan@kernel.org> wrote:
> > 
> > > When I compiled today, I met following warning.
> > > Correct it.
> > > 
> > > mm/memory.c: In function ___copy_page_range___:
> > > include/linux/mmu_notifier.h:235:38: warning: ___mmun_end___ may be used uninitialized in this function [-Wuninitialized]
> > > mm/memory.c:1043:16: note: ___mmun_end___ was declared here
> > > include/linux/mmu_notifier.h:235:38: warning: ___mmun_start___ may be used uninitialized in this function [-Wuninitialized]
> > > mm/memory.c:1042:16: note: ___mmun_start___ was declared here
> > >   LD      mm/built-in.o
> > > 
> > > Cc: Sagi Grimberg <sagig@mellanox.com>
> > > Cc: Haggai Eran <haggaie@mellanox.com>
> > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > > ---
> > >  mm/memory.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 10e9b38..d000449 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -1039,8 +1039,8 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> > >  	unsigned long next;
> > >  	unsigned long addr = vma->vm_start;
> > >  	unsigned long end = vma->vm_end;
> > > -	unsigned long mmun_start;	/* For mmu_notifiers */
> > > -	unsigned long mmun_end;		/* For mmu_notifiers */
> > > +	unsigned long uninitialized_var(mmun_start);	/* For mmu_notifiers */
> > > +	unsigned long uninitialized_var(mmun_end);	/* For mmu_notifiers */
> > >  	int ret;
> > >  
> > 
> > Well yes, but a) uninitialized_var is a bit ugly and has some potential
> > to hide real bugs and b) it's not completely obvious that
> > is_cow_mapping() is stable across those two calls.
> > 
> > I think a better approach is this:
> > 
> > --- a/mm/memory.c~mm-move-all-mmu-notifier-invocations-to-be-done-outside-the-pt-lock-fix-fix
> > +++ a/mm/memory.c
> > @@ -1041,6 +1041,7 @@ int copy_page_range(struct mm_struct *ds
> >  	unsigned long end = vma->vm_end;
> >  	unsigned long mmun_start;	/* For mmu_notifiers */
> >  	unsigned long mmun_end;		/* For mmu_notifiers */
> > +	bool is_cow;
> >  	int ret;
> >  
> >  	/*
> > @@ -1074,7 +1075,8 @@ int copy_page_range(struct mm_struct *ds
> >  	 * parent mm. And a permission downgrade will only happen if
> >  	 * is_cow_mapping() returns true.
> >  	 */
> > -	if (is_cow_mapping(vma->vm_flags)) {
> > +	is_cow = is_cow_mapping(vma->vm_flags);
> > +	if (is_cow) {
> >  		mmun_start = addr;
> >  		mmun_end   = end;
> >  		mmu_notifier_invalidate_range_start(src_mm, mmun_start,
> > @@ -1095,7 +1097,7 @@ int copy_page_range(struct mm_struct *ds
> >  		}
> >  	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
> >  
> > -	if (is_cow_mapping(vma->vm_flags))
> > +	if (is_cow)
> >  		mmu_notifier_invalidate_range_end(src_mm, mmun_start, mmun_end);
> >  	return ret;
> >  }
> > 
> > Unfortunately, my (old) versions of gcc are so stupid that they *still*
> > generate the warning even when the code is as obviously correct as this :(
> > 
> > Can you please test it with your compiler?
> 
> My compiler is stupidl, too. It still emit the samw warning when I apply
> your patch. In addition, I can't see any benefit of text size.

barrios@bbox:~/linux-mmotm$ gcc --version
gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3


-- 
Kind regards,
Minchan Kim

--
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>

[PATCH] mm: Fix compiler warning in copy_page_range

[Haggai Eran]

This patch fixes the warning about mmun_start/end used uninitialized in
copy_page_range, by initializing them regardless of whether the notifiers are
actually called.  It also makes sure the vm_flags in copy_page_range are only
read once.

Cc: Minchan Kim <minchan@kernel.org>
Signed-off-by: Haggai Eran <haggaie@mellanox.com>
---
 mm/memory.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 3c88368..423d214 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -712,7 +712,7 @@ static void print_bad_pte(struct vm_area_struct *vma, unsigned long addr,
 	add_taint(TAINT_BAD_PAGE);
 }
 
-static inline int is_cow_mapping(vm_flags_t flags)
+static inline bool is_cow_mapping(vm_flags_t flags)
 {
 	return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
 }
@@ -1041,6 +1041,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	unsigned long end = vma->vm_end;
 	unsigned long mmun_start;	/* For mmu_notifiers */
 	unsigned long mmun_end;		/* For mmu_notifiers */
+	bool is_cow;
 	int ret;
 
 	/*
@@ -1073,12 +1074,12 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	 * parent mm. And a permission downgrade will only happen if
 	 * is_cow_mapping() returns true.
 	 */
-	if (is_cow_mapping(vma->vm_flags)) {
-		mmun_start = addr;
-		mmun_end   = end;
+	is_cow = is_cow_mapping(vma->vm_flags);
+	mmun_start = addr;
+	mmun_end   = end;
+	if (is_cow)
 		mmu_notifier_invalidate_range_start(src_mm, mmun_start,
 						    mmun_end);
-	}
 
 	ret = 0;
 	dst_pgd = pgd_offset(dst_mm, addr);
@@ -1094,7 +1095,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		}
 	} while (dst_pgd++, src_pgd++, addr = next, addr != end);
 
-	if (is_cow_mapping(vma->vm_flags))
+	if (is_cow)
 		mmu_notifier_invalidate_range_end(src_mm, mmun_start,
 						  mmun_end);
 	return ret;
-- 
1.7.11.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: [PATCH] mm: Fix compiler warning in copy_page_range

[Minchan Kim]

On Mon, Sep 10, 2012 at 02:40:28PM +0300, Haggai Eran wrote:
> This patch fixes the warning about mmun_start/end used uninitialized in
> copy_page_range, by initializing them regardless of whether the notifiers are
> actually called.  It also makes sure the vm_flags in copy_page_range are only
> read once.
> 
> Cc: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Haggai Eran <haggaie@mellanox.com>
Reviewed-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

--
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>