mm/vmscan: drop unneeded assignment in kswapd()

PATCH Applied Lukas Bulwahn 2 participants 5 messages Oct 4, 2020 – Oct 5, 2020

Subsystems (4)

Memory management Kernel janitors MGLRU MM reclaim

lore

Share & tools

lore

Lukas Bulwahn

author

2020-10-04 12:58 UTC #0 lore raw

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

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 <[email protected]>

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

mm/vmscan.c +1 -1

index 466fc3144fff..130ee40c1255 100644
@@ -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);

Signature · 1 line

2.17.1

Mel Gorman

2020-10-04 19:24 UTC #1 lore raw

Quoted 14 lines · 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.

Quoted 6 lines

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 <[email protected]>

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.

Signature · 2 lines

Mel Gorman
SUSE Labs

Lukas Bulwahn

author

2020-10-05 06:58 UTC #2 lore raw

Quoted 20 lines · Mel Gorman

Quoted 19 lines · 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.

Quoted 9 lines

Quoted 1 line
Signed-off-by Lukas Bulwahn <[email protected]>
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

Mel Gorman

2020-10-05 07:56 UTC #3 lore raw

Quoted 36 lines · Lukas Bulwahn

Quoted 20 lines · Mel Gorman
Quoted 19 lines · 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.
Quoted 9 lines
Quoted 1 line
Signed-off-by Lukas Bulwahn <[email protected]>
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:

Quoted 9 lines

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.

Signature · 2 lines

Mel Gorman
SUSE Labs

Mel Gorman

Acked-by

2020-10-05 07:59 UTC #4 lore raw

In reply to Lukas Bulwahn

Quoted 36 lines · Lukas Bulwahn

Quoted 20 lines · Mel Gorman
Quoted 19 lines · 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.
Quoted 9 lines
Quoted 1 line
Signed-off-by Lukas Bulwahn <[email protected]>
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:

Quoted 9 lines

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 <[email protected]>

Signature · 2 lines

Mel Gorman
SUSE Labs