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

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

[Nick Desaulniers]

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

--
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/vmscan: fix unsequenced modification and access warning

[Michal Hocko]

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

--
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 v2] mm/vmscan: fix unsequenced modification and access warning

[Nick Desaulniers]

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

--
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 v2] mm/vmscan: fix unsequenced modification and access warning

[Michal Hocko]

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

--
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 v2] mm/vmscan: fix unsequenced modification and access warning

[Michal Hocko]

I have discussed this with our gcc guys and here is what they say:

On Wed 10-05-17 10:38:44, Michal Hocko wrote:
[...]
> 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?

: This is not the reason why the above is okay.  The following part:
:    { .a = c = ..., .b = c }
: is okay because there's a sequence point after each full expression, and 
: an initializer is a full expression, so there's a sequence point between 
: both initializers.  The following part:
:    { ... c = foo(c) ... }
: is okay as well, because there's a sequence point after evaluating all 
: arguments and before the actual call (otherwise the common 'i=next(i)' 
: idiom doesn't work).  So both constructs that potentially could be sources 
: of sequence point violations actually aren't and hence okay.  clangs 
: warning is invalid.

I guess it is worth reporting this to clang bugzilla. Could you take
care of that Nick?
-- 
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: [Patch v2] mm/vmscan: fix unsequenced modification and access warning

[Nick Desaulniers]

> I guess it is worth reporting this to clang bugzilla. Could you take
> care of that Nick?

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

--
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 v2] mm/vmscan: fix unsequenced modification and access warning

[Nick Desaulniers]

On Tue, May 16, 2017 at 10:27:46AM +0200, Michal Hocko wrote:
> I guess it is worth reporting this to clang bugzilla. Could you take
> care of that Nick?

>From https://bugs.llvm.org//show_bug.cgi?id=33065#c5
it seems that this is indeed a sequence bug in the previous version of
this code and not a compiler bug.  You can read that response for the
properly-cited wording but my TL;DR/understanding is for the given code:

struct foo bar = {
  .a = (c = 0),
  .b = c,
};

That the compiler is allowed to reorder the initializations of bar.a and
bar.b, so what the value of c here might not be what you expect.

--
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 v2] mm/vmscan: fix unsequenced modification and access warning

[Michal Hocko]

On Thu 25-05-17 21:43:43, Nick Desaulniers wrote:
> On Tue, May 16, 2017 at 10:27:46AM +0200, Michal Hocko wrote:
> > I guess it is worth reporting this to clang bugzilla. Could you take
> > care of that Nick?
> 
> >From https://bugs.llvm.org//show_bug.cgi?id=33065#c5
> it seems that this is indeed a sequence bug in the previous version of
> this code and not a compiler bug.  You can read that response for the
> properly-cited wording but my TL;DR/understanding is for the given code:
> 
> struct foo bar = {
>   .a = (c = 0),
>   .b = c,
> };
> 
> That the compiler is allowed to reorder the initializations of bar.a and
> bar.b, so what the value of c here might not be what you expect.

This is interesting because what I hear from our gcc people is something
different. I am not in a possition to argue here, though.
-- 
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: [PATCH] mm/vmscan: fix unsequenced modification and access warning

[Nick Desaulniers]

> 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

--
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/vmscan: fix unsequenced modification and access warning

[Michal Hocko]

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

--
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 v3] mm/vmscan: fix unsequenced modification and access warning

[Nick Desaulniers]

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

--
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/vmscan: fix unsequenced modification and access warning

[Nick Desaulniers]

Sorry to dig up an old thread but a coworker was asking about this
patch. This is essentially the code that landed in commit
f2f43e566a02a3bdde0a65e6a2e88d707c212a29 "mm/vmscan.c: fix unsequenced
modification and access warning".

Is .reclaim_idx still correct in the case of try_to_free_pages()?  It
looks like reclaim_idx is based on the original gfp_mask in
__node_reclaim(), but in try_to_free_pages() it looks like it may have
been based on current_gfp_context()? (The sequencing is kind of
ambiguous, thus fixed in my patch)

Was there a bug in the original try_to_free_pages() pre commit
f2f43e566a0, or is .reclaim_idx supposed to be different between
try_to_free_pages() and __node_reclaim()?

On Wed, May 10, 2017 at 12:15 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 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

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

[Michal Hocko]

On Wed 21-03-18 14:37:04, Nick Desaulniers wrote:
> Sorry to dig up an old thread but a coworker was asking about this
> patch. This is essentially the code that landed in commit
> f2f43e566a02a3bdde0a65e6a2e88d707c212a29 "mm/vmscan.c: fix unsequenced
> modification and access warning".
> 
> Is .reclaim_idx still correct in the case of try_to_free_pages()?

Yes, it gets initialized from the given gfp_mask. sc.gfp_mask might be
sllightly different but that doesn't change the reclaim_idx because we
only drop __GFP_{FS,IO} which do not have any zone modification effects.

> It
> looks like reclaim_idx is based on the original gfp_mask in
> __node_reclaim(), but in try_to_free_pages() it looks like it may have
> been based on current_gfp_context()? (The sequencing is kind of
> ambiguous, thus fixed in my patch)
> 
> Was there a bug in the original try_to_free_pages() pre commit
> f2f43e566a0, or is .reclaim_idx supposed to be different between
> try_to_free_pages() and __node_reclaim()?

I do not think there was any real bug.
-- 
Michal Hocko
SUSE Labs