* [PATCH] mm/vmscan: drop unneeded assignment in kswapd()
@ 2020-10-04 12:58 Lukas Bulwahn
2020-10-04 19:24 ` Mel Gorman
0 siblings, 1 reply; 5+ messages in thread
From: Lukas Bulwahn @ 2020-10-04 12:58 UTC (permalink / raw)
To: Mel Gorman, Andrew Morton, linux-mm
Cc: Vlastimil Babka, Michal Hocko, Nathan Chancellor,
Nick Desaulniers, linux-kernel, clang-built-linux,
kernel-janitors, linux-safety, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()
2020-10-04 12:58 [PATCH] mm/vmscan: drop unneeded assignment in kswapd() Lukas Bulwahn
@ 2020-10-04 19:24 ` Mel Gorman
2020-10-05 6:58 ` Lukas Bulwahn
0 siblings, 1 reply; 5+ messages in thread
From: Mel Gorman @ 2020-10-04 19:24 UTC (permalink / raw)
To: Lukas Bulwahn
Cc: Andrew Morton, linux-mm, Vlastimil Babka, Michal Hocko,
Nathan Chancellor, Nick Desaulniers, linux-kernel,
clang-built-linux, kernel-janitors, linux-safety
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()
2020-10-04 19:24 ` Mel Gorman
@ 2020-10-05 6:58 ` Lukas Bulwahn
2020-10-05 7:56 ` Mel Gorman
2020-10-05 7:59 ` Mel Gorman
0 siblings, 2 replies; 5+ messages in thread
From: Lukas Bulwahn @ 2020-10-05 6:58 UTC (permalink / raw)
To: Mel Gorman
Cc: Lukas Bulwahn, Andrew Morton, linux-mm, Vlastimil Babka,
Michal Hocko, Nathan Chancellor, Nick Desaulniers, linux-kernel,
clang-built-linux, kernel-janitors, linux-safety
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()
2020-10-05 6:58 ` Lukas Bulwahn
@ 2020-10-05 7:56 ` Mel Gorman
2020-10-05 7:59 ` Mel Gorman
1 sibling, 0 replies; 5+ messages in thread
From: Mel Gorman @ 2020-10-05 7:56 UTC (permalink / raw)
To: Lukas Bulwahn
Cc: Andrew Morton, linux-mm, Vlastimil Babka, Michal Hocko,
Nathan Chancellor, Nick Desaulniers, linux-kernel,
clang-built-linux, kernel-janitors, linux-safety
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm/vmscan: drop unneeded assignment in kswapd()
2020-10-05 6:58 ` Lukas Bulwahn
2020-10-05 7:56 ` Mel Gorman
@ 2020-10-05 7:59 ` Mel Gorman
1 sibling, 0 replies; 5+ messages in thread
From: Mel Gorman @ 2020-10-05 7:59 UTC (permalink / raw)
To: Lukas Bulwahn
Cc: Andrew Morton, linux-mm, Vlastimil Babka, Michal Hocko,
Nathan Chancellor, Nick Desaulniers, linux-kernel,
clang-built-linux, kernel-janitors, linux-safety
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-05 7:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-04 12:58 [PATCH] mm/vmscan: drop unneeded assignment in kswapd() Lukas Bulwahn
2020-10-04 19:24 ` Mel Gorman
2020-10-05 6:58 ` Lukas Bulwahn
2020-10-05 7:56 ` Mel Gorman
2020-10-05 7:59 ` Mel Gorman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).