[PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Lukas Bulwahn]

The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
kswapd sleeping prematurely due to mismatched classzone_idx") turned an
assignment to reclaim_order into a dead store, as in all further paths,
reclaim_order will be assigned again before it is used.

make clang-analyzer on x86_64 tinyconfig caught my attention with:

  mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
  used in the enclosing expression, the value is never actually read from
  'reclaim_order' [clang-analyzer-deadcode.DeadStores]

Compilers will detect this unneeded assignment and optimize this anyway.
So, the resulting binary is identical before and after this change.

Simplify the code and remove unneeded assignment to make clang-analyzer
happy.

No functional change. No change in binary code.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on current master and next-20201002

Mel, please ack.
Andrew, please pick this minor non-urgent clean-up patch.

I quickly confirmed that the binary did not change with this change to the
source code; The hash of mm/vmscan.o remained the same before and after
the change.

So, in my setup:
md5sum mm/vmscan.o
7da4675477f186263e36b726cc2b859d  mm/vmscan.o

linux-safety, please verify and validate this change.

 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 466fc3144fff..130ee40c1255 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3896,7 +3896,7 @@ static int kswapd(void *p)
 					highest_zoneidx);
 
 		/* Read the new order and highest_zoneidx */
-		alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);
+		alloc_order = READ_ONCE(pgdat->kswapd_order);
 		highest_zoneidx = kswapd_highest_zoneidx(pgdat,
 							highest_zoneidx);
 		WRITE_ONCE(pgdat->kswapd_order, 0);
-- 
2.17.1

[PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Lukas Bulwahn]

The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
kswapd sleeping prematurely due to mismatched classzone_idx") turned an
assignment to reclaim_order into a dead store, as in all further paths,
reclaim_order will be assigned again before it is used.

make clang-analyzer on x86_64 tinyconfig caught my attention with:

  mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
  used in the enclosing expression, the value is never actually read from
  'reclaim_order' [clang-analyzer-deadcode.DeadStores]

Compilers will detect this unneeded assignment and optimize this anyway.
So, the resulting binary is identical before and after this change.

Simplify the code and remove unneeded assignment to make clang-analyzer
happy.

No functional change. No change in binary code.

Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on current master and next-20201002

Mel, please ack.
Andrew, please pick this minor non-urgent clean-up patch.

I quickly confirmed that the binary did not change with this change to the
source code; The hash of mm/vmscan.o remained the same before and after
the change.

So, in my setup:
md5sum mm/vmscan.o
7da4675477f186263e36b726cc2b859d  mm/vmscan.o

linux-safety, please verify and validate this change.

 mm/vmscan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 466fc3144fff..130ee40c1255 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3896,7 +3896,7 @@ static int kswapd(void *p)
 					highest_zoneidx);
 
 		/* Read the new order and highest_zoneidx */
-		alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);
+		alloc_order = READ_ONCE(pgdat->kswapd_order);
 		highest_zoneidx = kswapd_highest_zoneidx(pgdat,
 							highest_zoneidx);
 		WRITE_ONCE(pgdat->kswapd_order, 0);
-- 
2.17.1

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Mel Gorman]

On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> assignment to reclaim_order into a dead store, as in all further paths,
> reclaim_order will be assigned again before it is used.
> 
> make clang-analyzer on x86_64 tinyconfig caught my attention with:
> 
>   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
>   used in the enclosing expression, the value is never actually read from
>   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> 
> Compilers will detect this unneeded assignment and optimize this anyway.
> So, the resulting binary is identical before and after this change.
> 
> Simplify the code and remove unneeded assignment to make clang-analyzer
> happy.
> 
> No functional change. No change in binary code.
> 
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

I'm not really keen on this. With the patch, reclaim_order can be passed
uninitialised to kswapd_try_to_sleep. While a sufficiently smart
compiler might be able to optimise how reclaim_order is used, it's not
guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
called functions could rely on reclaim_order being a valid value and
then introduce a subtle bug.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Mel Gorman]

On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> assignment to reclaim_order into a dead store, as in all further paths,
> reclaim_order will be assigned again before it is used.
> 
> make clang-analyzer on x86_64 tinyconfig caught my attention with:
> 
>   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
>   used in the enclosing expression, the value is never actually read from
>   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> 
> Compilers will detect this unneeded assignment and optimize this anyway.
> So, the resulting binary is identical before and after this change.
> 
> Simplify the code and remove unneeded assignment to make clang-analyzer
> happy.
> 
> No functional change. No change in binary code.
> 
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>

I'm not really keen on this. With the patch, reclaim_order can be passed
uninitialised to kswapd_try_to_sleep. While a sufficiently smart
compiler might be able to optimise how reclaim_order is used, it's not
guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
called functions could rely on reclaim_order being a valid value and
then introduce a subtle bug.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Lukas Bulwahn]

On Sun, 4 Oct 2020, Mel Gorman wrote:

> On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > assignment to reclaim_order into a dead store, as in all further paths,
> > reclaim_order will be assigned again before it is used.
> > 
> > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > 
> >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> >   used in the enclosing expression, the value is never actually read from
> >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > 
> > Compilers will detect this unneeded assignment and optimize this anyway.
> > So, the resulting binary is identical before and after this change.
> > 
> > Simplify the code and remove unneeded assignment to make clang-analyzer
> > happy.
> > 
> > No functional change. No change in binary code.
> > 
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> 
> I'm not really keen on this. With the patch, reclaim_order can be passed
> uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> compiler might be able to optimise how reclaim_order is used, it's not
> guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> called functions could rely on reclaim_order being a valid value and
> then introduce a subtle bug.
>

Just for my own understanding:

How would you see reclaim_order being passed unitialised to 
kswapd_try_to_sleep?

From kswapd() entry, any path must reach the line

  alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);

before kswap_try_to_sleep(...).

Then it reads back the order into alloc_order and reclaim_order
and resets pgdat->kswapd to 0.
I argue that the second store to reclaim_order is not used.

Path kthread_should_stop() is true:
Then, it either exits and does not use those temporary values, 
reclaim_order and alloc_order, at all.

Path try_to_freeze() is true:
It goes back to the beginning of the loop and repeats reading alloc_order 
and reclaim_order after the reset to 0, and then passes that to 
kswapd_try_to_sleep(...). Previous reclaim_order is not used.

So, the previous store to alloc_order and reclaim_order is lost.
(Is that intentional?) 

Path try_to_freeze() is false:
We call trace_mm_vmscan_kswapd_wake with alloc_order but not with 
reclaim_order. reclaim_order is set by the return of balance_pgdat(...);
So, the previous reclaim_order is again not used.

The diff in the patch might be a bit small, but we are looking at the 
second assignment after kswapd_try_to_sleep(...), not the first assignment 
that just looks the same.


Lukas

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Lukas Bulwahn]

On Sun, 4 Oct 2020, Mel Gorman wrote:

> On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > assignment to reclaim_order into a dead store, as in all further paths,
> > reclaim_order will be assigned again before it is used.
> > 
> > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > 
> >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> >   used in the enclosing expression, the value is never actually read from
> >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > 
> > Compilers will detect this unneeded assignment and optimize this anyway.
> > So, the resulting binary is identical before and after this change.
> > 
> > Simplify the code and remove unneeded assignment to make clang-analyzer
> > happy.
> > 
> > No functional change. No change in binary code.
> > 
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> 
> I'm not really keen on this. With the patch, reclaim_order can be passed
> uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> compiler might be able to optimise how reclaim_order is used, it's not
> guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> called functions could rely on reclaim_order being a valid value and
> then introduce a subtle bug.
>

Just for my own understanding:

How would you see reclaim_order being passed unitialised to 
kswapd_try_to_sleep?

From kswapd() entry, any path must reach the line

  alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);

before kswap_try_to_sleep(...).

Then it reads back the order into alloc_order and reclaim_order
and resets pgdat->kswapd to 0.
I argue that the second store to reclaim_order is not used.

Path kthread_should_stop() is true:
Then, it either exits and does not use those temporary values, 
reclaim_order and alloc_order, at all.

Path try_to_freeze() is true:
It goes back to the beginning of the loop and repeats reading alloc_order 
and reclaim_order after the reset to 0, and then passes that to 
kswapd_try_to_sleep(...). Previous reclaim_order is not used.

So, the previous store to alloc_order and reclaim_order is lost.
(Is that intentional?) 

Path try_to_freeze() is false:
We call trace_mm_vmscan_kswapd_wake with alloc_order but not with 
reclaim_order. reclaim_order is set by the return of balance_pgdat(...);
So, the previous reclaim_order is again not used.

The diff in the patch might be a bit small, but we are looking at the 
second assignment after kswapd_try_to_sleep(...), not the first assignment 
that just looks the same.


Lukas

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Lukas Bulwahn]

On Sun, 4 Oct 2020, Mel Gorman wrote:

> On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > assignment to reclaim_order into a dead store, as in all further paths,
> > reclaim_order will be assigned again before it is used.
> > 
> > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > 
> >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> >   used in the enclosing expression, the value is never actually read from
> >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > 
> > Compilers will detect this unneeded assignment and optimize this anyway.
> > So, the resulting binary is identical before and after this change.
> > 
> > Simplify the code and remove unneeded assignment to make clang-analyzer
> > happy.
> > 
> > No functional change. No change in binary code.
> > 
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> 
> I'm not really keen on this. With the patch, reclaim_order can be passed
> uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> compiler might be able to optimise how reclaim_order is used, it's not
> guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> called functions could rely on reclaim_order being a valid value and
> then introduce a subtle bug.
>

Just for my own understanding:

How would you see reclaim_order being passed unitialised to 
kswapd_try_to_sleep?

From kswapd() entry, any path must reach the line

  alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);

before kswap_try_to_sleep(...).

Then it reads back the order into alloc_order and reclaim_order
and resets pgdat->kswapd to 0.
I argue that the second store to reclaim_order is not used.

Path kthread_should_stop() is true:
Then, it either exits and does not use those temporary values, 
reclaim_order and alloc_order, at all.

Path try_to_freeze() is true:
It goes back to the beginning of the loop and repeats reading alloc_order 
and reclaim_order after the reset to 0, and then passes that to 
kswapd_try_to_sleep(...). Previous reclaim_order is not used.

So, the previous store to alloc_order and reclaim_order is lost.
(Is that intentional?) 

Path try_to_freeze() is false:
We call trace_mm_vmscan_kswapd_wake with alloc_order but not with 
reclaim_order. reclaim_order is set by the return of balance_pgdat(...);
So, the previous reclaim_order is again not used.

The diff in the patch might be a bit small, but we are looking at the 
second assignment after kswapd_try_to_sleep(...), not the first assignment 
that just looks the same.


Lukas

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Mel Gorman]

On Mon, Oct 05, 2020 at 08:58:53AM +0200, Lukas Bulwahn wrote:
> 
> 
> On Sun, 4 Oct 2020, Mel Gorman wrote:
> 
> > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > > assignment to reclaim_order into a dead store, as in all further paths,
> > > reclaim_order will be assigned again before it is used.
> > > 
> > > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > > 
> > >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> > >   used in the enclosing expression, the value is never actually read from
> > >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > > 
> > > Compilers will detect this unneeded assignment and optimize this anyway.
> > > So, the resulting binary is identical before and after this change.
> > > 
> > > Simplify the code and remove unneeded assignment to make clang-analyzer
> > > happy.
> > > 
> > > No functional change. No change in binary code.
> > > 
> > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > 
> > I'm not really keen on this. With the patch, reclaim_order can be passed
> > uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> > compiler might be able to optimise how reclaim_order is used, it's not
> > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> > called functions could rely on reclaim_order being a valid value and
> > then introduce a subtle bug.
> >
> 
> Just for my own understanding:
> 
> How would you see reclaim_order being passed unitialised to 
> kswapd_try_to_sleep?
> 
> From kswapd() entry, any path must reach the line
> 
>   alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);
> 
> before kswap_try_to_sleep(...).
> 

After your patch, the code is

	unsigned int alloc_order, reclaim_order;
	...

	for ( ; ; ) {
                alloc_order = READ_ONCE(pgdat->kswapd_order);
                highest_zoneidx = kswapd_highest_zoneidx(pgdat,
                                                        highest_zoneidx);

kswapd_try_sleep:
                kswapd_try_to_sleep(pgdat, alloc_order, reclaim_order,
                                        highest_zoneidx);
	...
		reclaim_order = balance_pgdat(pgdat, alloc_order,
                                                highest_zoneidx);

reclaim_order is declared, not initialised at the start of the loop and
passed into kswapd_try_to_sleep. There is a sequence where it is not used
so it does not matter but it depends on the compiler figuring that out.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Mel Gorman]

On Mon, Oct 05, 2020 at 08:58:53AM +0200, Lukas Bulwahn wrote:
> 
> 
> On Sun, 4 Oct 2020, Mel Gorman wrote:
> 
> > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > > assignment to reclaim_order into a dead store, as in all further paths,
> > > reclaim_order will be assigned again before it is used.
> > > 
> > > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > > 
> > >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> > >   used in the enclosing expression, the value is never actually read from
> > >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > > 
> > > Compilers will detect this unneeded assignment and optimize this anyway.
> > > So, the resulting binary is identical before and after this change.
> > > 
> > > Simplify the code and remove unneeded assignment to make clang-analyzer
> > > happy.
> > > 
> > > No functional change. No change in binary code.
> > > 
> > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > 
> > I'm not really keen on this. With the patch, reclaim_order can be passed
> > uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> > compiler might be able to optimise how reclaim_order is used, it's not
> > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> > called functions could rely on reclaim_order being a valid value and
> > then introduce a subtle bug.
> >
> 
> Just for my own understanding:
> 
> How would you see reclaim_order being passed unitialised to 
> kswapd_try_to_sleep?
> 
> From kswapd() entry, any path must reach the line
> 
>   alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);
> 
> before kswap_try_to_sleep(...).
> 

After your patch, the code is

	unsigned int alloc_order, reclaim_order;
	...

	for ( ; ; ) {
                alloc_order = READ_ONCE(pgdat->kswapd_order);
                highest_zoneidx = kswapd_highest_zoneidx(pgdat,
                                                        highest_zoneidx);

kswapd_try_sleep:
                kswapd_try_to_sleep(pgdat, alloc_order, reclaim_order,
                                        highest_zoneidx);
	...
		reclaim_order = balance_pgdat(pgdat, alloc_order,
                                                highest_zoneidx);

reclaim_order is declared, not initialised at the start of the loop and
passed into kswapd_try_to_sleep. There is a sequence where it is not used
so it does not matter but it depends on the compiler figuring that out.

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Mel Gorman]

On Mon, Oct 05, 2020 at 08:58:53AM +0200, Lukas Bulwahn wrote:
> 
> 
> On Sun, 4 Oct 2020, Mel Gorman wrote:
> 
> > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > > assignment to reclaim_order into a dead store, as in all further paths,
> > > reclaim_order will be assigned again before it is used.
> > > 
> > > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > > 
> > >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> > >   used in the enclosing expression, the value is never actually read from
> > >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > > 
> > > Compilers will detect this unneeded assignment and optimize this anyway.
> > > So, the resulting binary is identical before and after this change.
> > > 
> > > Simplify the code and remove unneeded assignment to make clang-analyzer
> > > happy.
> > > 
> > > No functional change. No change in binary code.
> > > 
> > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > 
> > I'm not really keen on this. With the patch, reclaim_order can be passed
> > uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> > compiler might be able to optimise how reclaim_order is used, it's not
> > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> > called functions could rely on reclaim_order being a valid value and
> > then introduce a subtle bug.
> >
> 
> Just for my own understanding:
> 
> How would you see reclaim_order being passed unitialised to 
> kswapd_try_to_sleep?
> 
> From kswapd() entry, any path must reach the line
> 
>   alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);
> 
> before kswap_try_to_sleep(...).
> 

Bah, I misread the patch because I'm an idiot.

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs

Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()

[Mel Gorman]

On Mon, Oct 05, 2020 at 08:58:53AM +0200, Lukas Bulwahn wrote:
> 
> 
> On Sun, 4 Oct 2020, Mel Gorman wrote:
> 
> > On Sun, Oct 04, 2020 at 02:58:27PM +0200, Lukas Bulwahn wrote:
> > > The refactoring to kswapd() in commit e716f2eb24de ("mm, vmscan: prevent
> > > kswapd sleeping prematurely due to mismatched classzone_idx") turned an
> > > assignment to reclaim_order into a dead store, as in all further paths,
> > > reclaim_order will be assigned again before it is used.
> > > 
> > > make clang-analyzer on x86_64 tinyconfig caught my attention with:
> > > 
> > >   mm/vmscan.c: warning: Although the value stored to 'reclaim_order' is
> > >   used in the enclosing expression, the value is never actually read from
> > >   'reclaim_order' [clang-analyzer-deadcode.DeadStores]
> > > 
> > > Compilers will detect this unneeded assignment and optimize this anyway.
> > > So, the resulting binary is identical before and after this change.
> > > 
> > > Simplify the code and remove unneeded assignment to make clang-analyzer
> > > happy.
> > > 
> > > No functional change. No change in binary code.
> > > 
> > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > 
> > I'm not really keen on this. With the patch, reclaim_order can be passed
> > uninitialised to kswapd_try_to_sleep. While a sufficiently smart
> > compiler might be able to optimise how reclaim_order is used, it's not
> > guaranteed either. Similarly, a change in kswapd_try_to_sleep and its
> > called functions could rely on reclaim_order being a valid value and
> > then introduce a subtle bug.
> >
> 
> Just for my own understanding:
> 
> How would you see reclaim_order being passed unitialised to 
> kswapd_try_to_sleep?
> 
> From kswapd() entry, any path must reach the line
> 
>   alloc_order = reclaim_order = READ_ONCE(pgdat->kswapd_order);
> 
> before kswap_try_to_sleep(...).
> 

Bah, I misread the patch because I'm an idiot.

Acked-by: Mel Gorman <mgorman@techsingularity.net>

-- 
Mel Gorman
SUSE Labs