Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1638594 > unrolled thread

[PATCH] mm/vmscan: fix unsequenced modification and access warning

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm/vmscan: fix unsequenced modification and access warning Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-05-10 09:00 +0200
    Re: [PATCH] mm/vmscan: fix unsequenced modification and access  warning Michal Hocko <mhocko@kernel.org> - 2017-05-10 09:20 +0200
      [Patch v2] mm/vmscan: fix unsequenced modification and access warning Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-05-10 10:30 +0200
        Re: [Patch v2] mm/vmscan: fix unsequenced modification and access  warning Michal Hocko <mhocko@kernel.org> - 2017-05-10 10:40 +0200
      Re: [PATCH] mm/vmscan: fix unsequenced modification and access  warning Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-05-10 10:50 +0200
        Re: [PATCH] mm/vmscan: fix unsequenced modification and access  warning Michal Hocko <mhocko@kernel.org> - 2017-05-10 11:30 +0200
          [Patch v3] mm/vmscan: fix unsequenced modification and access warning Nick Desaulniers <nick.desaulniers@gmail.com> - 2017-05-10 17:50 +0200

#1638594 — [PATCH] mm/vmscan: fix unsequenced modification and access warning

Clang flags this file with the -Wunsequenced error that GCC does not
have.

unsequenced modification and access to 'gfp_mask'

It seems that gfp_mask is both read and written without a sequence point
in between, which is undefined behavior.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 mm/vmscan.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4e7ed65842af..74785908822c 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	unsigned long nr_reclaimed;
 	struct scan_control sc = {
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.reclaim_idx = gfp_zone(gfp_mask),
 		.order = order,
 		.nodemask = nodemask,
@@ -2968,6 +2968,8 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 		.may_swap = 1,
 	};
 
+	gfp_mask = sc.gfp_mask;
+
 	/*
 	 * Do not enter reclaim if fatal signal was delivered while throttled.
 	 * 1 is returned so that the page allocator does not OOM kill at this
-- 
2.11.0

[toc] | [next] | [standalone]

#1638603 — Re: [PATCH] mm/vmscan: fix unsequenced modification and access warning

On Tue 09-05-17 23:53:28, Nick Desaulniers wrote:
> Clang flags this file with the -Wunsequenced error that GCC does not
> have.
> 
> unsequenced modification and access to 'gfp_mask'
> 
> It seems that gfp_mask is both read and written without a sequence point
> in between, which is undefined behavior.

Hmm. This is rather news to me. I thought that a = foo(a) is perfectly
valid. Same as a = b = c where c = foo(b) or is the problem in the
following .reclaim_idx = gfp_zone(gfp_mask) initialization? If that is
the case then the current code is OKish because gfp_zone doesn't depend
on the gfp_mask modification. It is messy, right, but works as expected.

Anyway, we have a similar construct __node_reclaim

If you really want to change this code, and I would agree it would be
slightly less tricky, then I would suggest doing something like the
following instead
---
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 5ebf468c5429..ba4b695e810e 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2965,7 +2965,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	unsigned long nr_reclaimed;
 	struct scan_control sc = {
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.reclaim_idx = gfp_zone(gfp_mask),
 		.order = order,
 		.nodemask = nodemask,
@@ -2980,12 +2980,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	 * 1 is returned so that the page allocator does not OOM kill at this
 	 * point.
 	 */
-	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
+	if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,
 				sc.may_writepage,
-				gfp_mask,
+				sc.gfp_mask,
 				sc.reclaim_idx);
 
 	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
@@ -3772,17 +3772,16 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	const unsigned long nr_pages = 1 << order;
 	struct task_struct *p = current;
 	struct reclaim_state reclaim_state;
-	int classzone_idx = gfp_zone(gfp_mask);
 	unsigned int noreclaim_flag;
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.order = order,
 		.priority = NODE_RECLAIM_PRIORITY,
 		.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
 		.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
 		.may_swap = 1,
-		.reclaim_idx = classzone_idx,
+		.reclaim_idx = gfp_znoe(gfp_mask),
 	};
 
 	cond_resched();
@@ -3793,7 +3792,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	 */
 	noreclaim_flag = memalloc_noreclaim_save();
 	p->flags |= PF_SWAPWRITE;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	lockdep_set_current_reclaim_state(sc.gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1638651 — [Patch v2] mm/vmscan: fix unsequenced modification and access warning

Clang flags this file with the -Wunsequenced error that GCC does not
have.

unsequenced modification and access to 'gfp_mask'

It seems that gfp_mask is both read and written without a sequence point
in between, which is undefined behavior.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
Changes in v2:
- don't assign back to gfp_mask, reuse sc.gfp_mask
- initialize reclaim_idx directly, without classzone_idx

 mm/vmscan.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4e7ed65842af..d32c42d17935 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	unsigned long nr_reclaimed;
 	struct scan_control sc = {
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.reclaim_idx = gfp_zone(gfp_mask),
 		.order = order,
 		.nodemask = nodemask,
@@ -2973,12 +2973,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	 * 1 is returned so that the page allocator does not OOM kill at this
 	 * point.
 	 */
-	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
+	if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,
 				sc.may_writepage,
-				gfp_mask,
+				sc.gfp_mask,
 				sc.reclaim_idx);
 
 	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
@@ -3763,16 +3763,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	const unsigned long nr_pages = 1 << order;
 	struct task_struct *p = current;
 	struct reclaim_state reclaim_state;
-	int classzone_idx = gfp_zone(gfp_mask);
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.order = order,
 		.priority = NODE_RECLAIM_PRIORITY,
 		.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
 		.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
 		.may_swap = 1,
-		.reclaim_idx = classzone_idx,
+		.reclaim_idx = gfp_zone(gfp_mask),
 	};
 
 	cond_resched();
@@ -3782,7 +3781,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	 * and RECLAIM_UNMAP.
 	 */
 	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	lockdep_set_current_reclaim_state(sc.gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1638655 — Re: [Patch v2] mm/vmscan: fix unsequenced modification and access warning

On Wed 10-05-17 01:27:34, Nick Desaulniers wrote:
> Clang flags this file with the -Wunsequenced error that GCC does not
> have.
> 
> unsequenced modification and access to 'gfp_mask'
> 
> It seems that gfp_mask is both read and written without a sequence point
> in between, which is undefined behavior.
> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>

I will definitely not object to the patch as the code is cleaner and less tricky.
You can add
Acked-by: Michal Hocko <mhocko@suse.com>

But I
still do not understand which part of the code is undefined and why. My
reading and understanding of the C specification is that
struct A {
	int a;
	int b;
};

struct A f = { .a = c = foo(c), .b = c};

as long as foo(c) doesn't have any side effects because because .a is
initialized before b and the assignment ordering will make sure that c
is initialized before a.

6.7.8 par 19 (ISO/IEC 9899)
19 The initialization shall occur in initializer list order, each
   initializer provided for a particular subobject overriding any
   previously listed initializer for the same subobject; all subobjects
   that are not initialized explicitly shall be initialized implicitly
   the same as objects that have static storage duration.

So is my understanding of the specification wrong or is this a bug in
-Wunsequenced in Clang?

> ---
> Changes in v2:
> - don't assign back to gfp_mask, reuse sc.gfp_mask
> - initialize reclaim_idx directly, without classzone_idx
> 
>  mm/vmscan.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 4e7ed65842af..d32c42d17935 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
>  	unsigned long nr_reclaimed;
>  	struct scan_control sc = {
>  		.nr_to_reclaim = SWAP_CLUSTER_MAX,
> -		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
> +		.gfp_mask = current_gfp_context(gfp_mask),
>  		.reclaim_idx = gfp_zone(gfp_mask),
>  		.order = order,
>  		.nodemask = nodemask,
> @@ -2973,12 +2973,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
>  	 * 1 is returned so that the page allocator does not OOM kill at this
>  	 * point.
>  	 */
> -	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
> +	if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
>  		return 1;
>  
>  	trace_mm_vmscan_direct_reclaim_begin(order,
>  				sc.may_writepage,
> -				gfp_mask,
> +				sc.gfp_mask,
>  				sc.reclaim_idx);
>  
>  	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
> @@ -3763,16 +3763,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
>  	const unsigned long nr_pages = 1 << order;
>  	struct task_struct *p = current;
>  	struct reclaim_state reclaim_state;
> -	int classzone_idx = gfp_zone(gfp_mask);
>  	struct scan_control sc = {
>  		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
> -		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
> +		.gfp_mask = current_gfp_context(gfp_mask),
>  		.order = order,
>  		.priority = NODE_RECLAIM_PRIORITY,
>  		.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
>  		.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
>  		.may_swap = 1,
> -		.reclaim_idx = classzone_idx,
> +		.reclaim_idx = gfp_zone(gfp_mask),
>  	};
>  
>  	cond_resched();
> @@ -3782,7 +3781,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
>  	 * and RECLAIM_UNMAP.
>  	 */
>  	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
> -	lockdep_set_current_reclaim_state(gfp_mask);
> +	lockdep_set_current_reclaim_state(sc.gfp_mask);
>  	reclaim_state.reclaimed_slab = 0;
>  	p->reclaim_state = &reclaim_state;
>  
> -- 
> 2.11.0
> 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1638674 — Re: [PATCH] mm/vmscan: fix unsequenced modification and access warning

> You can add

Something that's not clear to me when advised to add, should I be
uploading a v3 with your acked by? I think I got that wrong the last
time I asked (which was my first patch to Linux).

> But I still do not understand which part of the code is undefined and
> why.

It's not immediately clear to me either, but it's super later here...

>  is this a bug in -Wunsequenced in Clang

Possibly, I think I already found one earlier tonight.

https://bugs.llvm.org/show_bug.cgi?id=32985

Tomorrow, I'll try to cut down a test case to see if this is indeed a
compiler bug.  Would you like me to change the commit message to call
this just a simple clean up, in the meantime?

Thanks,
~Nick

[toc] | [prev] | [next] | [standalone]

#1638711 — Re: [PATCH] mm/vmscan: fix unsequenced modification and access warning

On Wed 10-05-17 01:46:03, Nick Desaulniers wrote:
> > You can add
> 
> Something that's not clear to me when advised to add, should I be
> uploading a v3 with your acked by? I think I got that wrong the last
> time I asked (which was my first patch to Linux).

If there are no further changes to the patch/changelog then it is not
necessary. The maintainer usually just grabs ackes and reviewed-bys
from the list.

> > But I still do not understand which part of the code is undefined and
> > why.
> 
> It's not immediately clear to me either, but it's super later here...

I would really like to understand that...
 
> >  is this a bug in -Wunsequenced in Clang
> 
> Possibly, I think I already found one earlier tonight.
> 
> https://bugs.llvm.org/show_bug.cgi?id=32985

this seems unrelated. I would try to report this and clarify in the llvm
bugzilla.

> Tomorrow, I'll try to cut down a test case to see if this is indeed a
> compiler bug.  Would you like me to change the commit message to call
> this just a simple clean up, in the meantime?

I would go with the following wording.
"
Clang and its -Wunsequenced emits a warning
(PUT THE FULL WARNING HERE).

While it is not clear to me whether the initialization code violates the
specification (6.7.8 par 19 (ISO/IEC 9899) looks it disagrees) the code
is quite confusing and worth cleaning up anyway. Fix this by reusing
sc.gfp_mask rather than the updated input gfp_mask parameter.
"
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1638925 — [Patch v3] mm/vmscan: fix unsequenced modification and access warning

Clang and its -Wunsequenced emits a warning

mm/vmscan.c:2961:25: error: unsequenced modification and access to
'gfp_mask' [-Wunsequenced]
                .gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
                                      ^

While it is not clear to me whether the initialization code violates the
specification (6.7.8 par 19 (ISO/IEC 9899) looks like it disagrees) the
code is quite confusing and worth cleaning up anyway. Fix this by
reusing sc.gfp_mask rather than the updated input gfp_mask parameter.

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
Changes in v3:
- changed commit message
- added previous ack

Will file a bug with llvm later today

 mm/vmscan.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 4e7ed65842af..d32c42d17935 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2958,7 +2958,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	unsigned long nr_reclaimed;
 	struct scan_control sc = {
 		.nr_to_reclaim = SWAP_CLUSTER_MAX,
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.reclaim_idx = gfp_zone(gfp_mask),
 		.order = order,
 		.nodemask = nodemask,
@@ -2973,12 +2973,12 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
 	 * 1 is returned so that the page allocator does not OOM kill at this
 	 * point.
 	 */
-	if (throttle_direct_reclaim(gfp_mask, zonelist, nodemask))
+	if (throttle_direct_reclaim(sc.gfp_mask, zonelist, nodemask))
 		return 1;
 
 	trace_mm_vmscan_direct_reclaim_begin(order,
 				sc.may_writepage,
-				gfp_mask,
+				sc.gfp_mask,
 				sc.reclaim_idx);
 
 	nr_reclaimed = do_try_to_free_pages(zonelist, &sc);
@@ -3763,16 +3763,15 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	const unsigned long nr_pages = 1 << order;
 	struct task_struct *p = current;
 	struct reclaim_state reclaim_state;
-	int classzone_idx = gfp_zone(gfp_mask);
 	struct scan_control sc = {
 		.nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX),
-		.gfp_mask = (gfp_mask = current_gfp_context(gfp_mask)),
+		.gfp_mask = current_gfp_context(gfp_mask),
 		.order = order,
 		.priority = NODE_RECLAIM_PRIORITY,
 		.may_writepage = !!(node_reclaim_mode & RECLAIM_WRITE),
 		.may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP),
 		.may_swap = 1,
-		.reclaim_idx = classzone_idx,
+		.reclaim_idx = gfp_zone(gfp_mask),
 	};
 
 	cond_resched();
@@ -3782,7 +3781,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in
 	 * and RECLAIM_UNMAP.
 	 */
 	p->flags |= PF_MEMALLOC | PF_SWAPWRITE;
-	lockdep_set_current_reclaim_state(gfp_mask);
+	lockdep_set_current_reclaim_state(sc.gfp_mask);
 	reclaim_state.reclaimed_slab = 0;
 	p->reclaim_state = &reclaim_state;
 
-- 
2.11.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web