linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
@ 2026-02-13 12:38 Arnd Bergmann
  2026-02-13 16:58 ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2026-02-13 12:38 UTC (permalink / raw)
  To: Andrew Morton, Johannes Weiner, Axel Rasmussen, Yuanchu Xie
  Cc: Arnd Bergmann, Wei Xu, David Hildenbrand, Michal Hocko, Qi Zheng,
	Shakeel Butt, Lorenzo Stoakes, Baolin Wang, Kairui Song,
	Davidlohr Bueso, Koichiro Den, Jiayuan Chen, Bertrand Wlodarczyk,
	linux-mm, linux-kernel

From: Arnd Bergmann <arnd@arndb.de>

When the -fsanitize=bounds sanitizer is enabled, gcc-16 sometimes runs
into a corner case in the read_ctrl_pos() pos function, where it sees
possible undefined behavior from the 'tier' index overflowing, presumably
in the case that this was called with a negative tier:

In function 'get_tier_idx',
    inlined from 'isolate_folios' at mm/vmscan.c:4671:14:
mm/vmscan.c: In function 'isolate_folios':
mm/vmscan.c:4645:29: error: 'pv.refaulted' is used uninitialized [-Werror=uninitialized]

Part of the problem seems to be that read_ctrl_pos() has unusual calling
conventions since commit 37a260870f2c ("mm/mglru: rework type selection")
where passing MAX_NR_TIERS makes it accumulate all tiers but passing a
smaller positive number makes it read a single tier instead.

Avoid this case by splitting read_ctrl_pos() into two separate helpers
that each only do one of the two cases. This avoids the warning as far
as I can tell, and seems a bit easier to understand to me.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
This is currently the only such warning I get from gcc-16.0.1, and
none from any other version.

I'm not overly happy about having to work around it with a random
code chance, but hopefully the version I ended up with makes
sense regardless.
---
 mm/vmscan.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index eaf795c1cfb3..602c955d1f30 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -3159,20 +3159,15 @@ struct ctrl_pos {
 static void read_ctrl_pos(struct lruvec *lruvec, int type, int tier, int gain,
 			  struct ctrl_pos *pos)
 {
-	int i;
 	struct lru_gen_folio *lrugen = &lruvec->lrugen;
 	int hist = lru_hist_from_seq(lrugen->min_seq[type]);
 
 	pos->gain = gain;
-	pos->refaulted = pos->total = 0;
-
-	for (i = tier % MAX_NR_TIERS; i <= min(tier, MAX_NR_TIERS - 1); i++) {
-		pos->refaulted += lrugen->avg_refaulted[type][i] +
-				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
-		pos->total += lrugen->avg_total[type][i] +
-			      lrugen->protected[hist][type][i] +
-			      atomic_long_read(&lrugen->evicted[hist][type][i]);
-	}
+	pos->refaulted = lrugen->avg_refaulted[type][tier] +
+			 atomic_long_read(&lrugen->refaulted[hist][type][tier]);
+	pos->total = lrugen->avg_total[type][tier] +
+		     lrugen->protected[hist][type][tier] +
+		     atomic_long_read(&lrugen->evicted[hist][type][tier]);
 }
 
 static void reset_ctrl_pos(struct lruvec *lruvec, int type, bool carryover)
@@ -4640,6 +4635,24 @@ static int get_tier_idx(struct lruvec *lruvec, int type)
 	return tier - 1;
 }
 
+static void aggregate_ctrl_pos(struct lruvec *lruvec, int type, int gain,
+			       struct ctrl_pos *pos)
+{
+	struct lru_gen_folio *lrugen = &lruvec->lrugen;
+	int hist = lru_hist_from_seq(lrugen->min_seq[type]);
+
+	pos->gain = gain;
+	pos->refaulted = pos->total = 0;
+
+	for (int i = 0; i < MAX_NR_TIERS; i++) {
+		pos->refaulted += lrugen->avg_refaulted[type][i] +
+				  atomic_long_read(&lrugen->refaulted[hist][type][i]);
+		pos->total += lrugen->avg_total[type][i] +
+			      lrugen->protected[hist][type][i] +
+			      atomic_long_read(&lrugen->evicted[hist][type][i]);
+	}
+}
+
 static int get_type_to_scan(struct lruvec *lruvec, int swappiness)
 {
 	struct ctrl_pos sp, pv;
@@ -4653,8 +4666,8 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)
 	 * Compare the sum of all tiers of anon with that of file to determine
 	 * which type to scan.
 	 */
-	read_ctrl_pos(lruvec, LRU_GEN_ANON, MAX_NR_TIERS, swappiness, &sp);
-	read_ctrl_pos(lruvec, LRU_GEN_FILE, MAX_NR_TIERS, MAX_SWAPPINESS - swappiness, &pv);
+	aggregate_ctrl_pos(lruvec, LRU_GEN_ANON, swappiness, &sp);
+	aggregate_ctrl_pos(lruvec, LRU_GEN_FILE, MAX_SWAPPINESS - swappiness, &pv);
 
 	return positive_ctrl_err(&sp, &pv);
 }
-- 
2.39.5



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
  2026-02-13 12:38 [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning Arnd Bergmann
@ 2026-02-13 16:58 ` Andrew Morton
  2026-02-13 17:07   ` Arnd Bergmann
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2026-02-13 16:58 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Johannes Weiner, Axel Rasmussen, Yuanchu Xie, Arnd Bergmann,
	Wei Xu, David Hildenbrand, Michal Hocko, Qi Zheng, Shakeel Butt,
	Lorenzo Stoakes, Baolin Wang, Kairui Song, Davidlohr Bueso,
	Koichiro Den, Jiayuan Chen, Bertrand Wlodarczyk, linux-mm,
	linux-kernel

On Fri, 13 Feb 2026 13:38:56 +0100 Arnd Bergmann <arnd@kernel.org> wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> When the -fsanitize=bounds sanitizer is enabled,

Is this an option in current kernels?

> gcc-16 sometimes runs
> into a corner case in the read_ctrl_pos() pos function, where it sees
> possible undefined behavior from the 'tier' index overflowing, presumably
> in the case that this was called with a negative tier:
> 
> In function 'get_tier_idx',
>     inlined from 'isolate_folios' at mm/vmscan.c:4671:14:
> mm/vmscan.c: In function 'isolate_folios':
> mm/vmscan.c:4645:29: error: 'pv.refaulted' is used uninitialized [-Werror=uninitialized]
> 
> Part of the problem seems to be that read_ctrl_pos() has unusual calling
> conventions since commit 37a260870f2c ("mm/mglru: rework type selection")
> where passing MAX_NR_TIERS makes it accumulate all tiers but passing a
> smaller positive number makes it read a single tier instead.
> 
> Avoid this case by splitting read_ctrl_pos() into two separate helpers
> that each only do one of the two cases. This avoids the warning as far
> as I can tell, and seems a bit easier to understand to me.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> This is currently the only such warning I get from gcc-16.0.1, and
> none from any other version.
> 
> I'm not overly happy about having to work around it with a random
> code chance, but hopefully the version I ended up with makes
> sense regardless.

Seems a large change just to squish a compiler warning.  People might
prefer a simple 

-	struct ctrl_pos sp, pv;
+	struct ctrl_pos sp, pv = {};

?




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
  2026-02-13 16:58 ` Andrew Morton
@ 2026-02-13 17:07   ` Arnd Bergmann
  2026-02-13 17:23     ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2026-02-13 17:07 UTC (permalink / raw)
  To: Andrew Morton, Arnd Bergmann
  Cc: Johannes Weiner, Axel Rasmussen, Yuanchu Xie, Wei Xu,
	David Hildenbrand (Red Hat),
	Michal Hocko, Qi Zheng, Shakeel Butt, Lorenzo Stoakes,
	Baolin Wang, Kairui Song, Davidlohr Bueso, Koichiro Den,
	Jiayuan Chen, Bertrand Wlodarczyk, linux-mm, linux-kernel

On Fri, Feb 13, 2026, at 17:58, Andrew Morton wrote:
> On Fri, 13 Feb 2026 13:38:56 +0100 Arnd Bergmann <arnd@kernel.org> wrote:
>
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> When the -fsanitize=bounds sanitizer is enabled,
>
> Is this an option in current kernels?

Yes, this is CONFIG_UBSAN_ARRAY_BOUNDS. The actual warning
only shows up in some configurations with that, so either there
is some other dependency, or an element of chance based on gcc
optimizations.

> Seems a large change just to squish a compiler warning.  People might
> prefer a simple 
>
> -	struct ctrl_pos sp, pv;
> +	struct ctrl_pos sp, pv = {};

Right, that would clearly also shut up the warning.

To me this seems less intuitive without an extra comment,
since read_ctrl_pos() is meant to initialize the entire
struct, but please pick whichever you find most readable
here.

      Arnd


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
  2026-02-13 17:07   ` Arnd Bergmann
@ 2026-02-13 17:23     ` Andrew Morton
  2026-02-17 20:55       ` Yuanchu Xie
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2026-02-13 17:23 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Arnd Bergmann, Johannes Weiner, Axel Rasmussen, Yuanchu Xie,
	Wei Xu, David Hildenbrand (Red Hat),
	Michal Hocko, Qi Zheng, Shakeel Butt, Lorenzo Stoakes,
	Baolin Wang, Kairui Song, Davidlohr Bueso, Koichiro Den,
	Jiayuan Chen, Bertrand Wlodarczyk, linux-mm, linux-kernel

On Fri, 13 Feb 2026 18:07:04 +0100 "Arnd Bergmann" <arnd@arndb.de> wrote:

> On Fri, Feb 13, 2026, at 17:58, Andrew Morton wrote:
> > On Fri, 13 Feb 2026 13:38:56 +0100 Arnd Bergmann <arnd@kernel.org> wrote:
> >
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> When the -fsanitize=bounds sanitizer is enabled,
> >
> > Is this an option in current kernels?
> 
> Yes, this is CONFIG_UBSAN_ARRAY_BOUNDS. The actual warning
> only shows up in some configurations with that, so either there
> is some other dependency, or an element of chance based on gcc
> optimizations.

OK, I'll put a cc:stable on this, as people will want to compile older
kernels with gcc-16.

Aiming for upstreaming into 7.1-rc1 unless it's more urgent than I
think.

> > Seems a large change just to squish a compiler warning.  People might
> > prefer a simple 
> >
> > -	struct ctrl_pos sp, pv;
> > +	struct ctrl_pos sp, pv = {};
> 
> Right, that would clearly also shut up the warning.
> 
> To me this seems less intuitive without an extra comment,
> since read_ctrl_pos() is meant to initialize the entire
> struct, but please pick whichever you find most readable
> here.

Let's see what the MGLRU maintainers have to say.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
  2026-02-13 17:23     ` Andrew Morton
@ 2026-02-17 20:55       ` Yuanchu Xie
  2026-02-17 21:22         ` Axel Rasmussen
  0 siblings, 1 reply; 6+ messages in thread
From: Yuanchu Xie @ 2026-02-17 20:55 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Arnd Bergmann, Arnd Bergmann, Johannes Weiner, Axel Rasmussen,
	Wei Xu, David Hildenbrand (Red Hat),
	Michal Hocko, Qi Zheng, Shakeel Butt, Lorenzo Stoakes,
	Baolin Wang, Kairui Song, Davidlohr Bueso, Koichiro Den,
	Jiayuan Chen, Bertrand Wlodarczyk, linux-mm, linux-kernel

Hi Andrew and Arnd,

On Fri, Feb 13, 2026 at 11:23 AM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Fri, 13 Feb 2026 18:07:04 +0100 "Arnd Bergmann" <arnd@arndb.de> wrote:
>
> > On Fri, Feb 13, 2026, at 17:58, Andrew Morton wrote:
> > > On Fri, 13 Feb 2026 13:38:56 +0100 Arnd Bergmann <arnd@kernel.org> wrote:
> > >
> > >> From: Arnd Bergmann <arnd@arndb.de>
> > >>
> > >> When the -fsanitize=bounds sanitizer is enabled,
> > >
> > > Is this an option in current kernels?
> >
> > Yes, this is CONFIG_UBSAN_ARRAY_BOUNDS. The actual warning
> > only shows up in some configurations with that, so either there
> > is some other dependency, or an element of chance based on gcc
> > optimizations.
>
> OK, I'll put a cc:stable on this, as people will want to compile older
> kernels with gcc-16.
>
> Aiming for upstreaming into 7.1-rc1 unless it's more urgent than I
> think.
>
> > > Seems a large change just to squish a compiler warning.  People might
> > > prefer a simple
> > >
> > > -   struct ctrl_pos sp, pv;
> > > +   struct ctrl_pos sp, pv = {};
> >
> > Right, that would clearly also shut up the warning.
> >
> > To me this seems less intuitive without an extra comment,
> > since read_ctrl_pos() is meant to initialize the entire
> > struct, but please pick whichever you find most readable
> > here.
>
> Let's see what the MGLRU maintainers have to say.

I went over the various cases of read_ctrl_pos and couldn't find
anything wrong (let me know if I'm mistaken), so this seems like a
mild compiler bug we're trying to work around. Given it's one single
version of gcc, I'm a fan of the simple `struct ctrl_pos sp, pv = {};`
with a comment.

Thanks,
Yuanchu


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
  2026-02-17 20:55       ` Yuanchu Xie
@ 2026-02-17 21:22         ` Axel Rasmussen
  0 siblings, 0 replies; 6+ messages in thread
From: Axel Rasmussen @ 2026-02-17 21:22 UTC (permalink / raw)
  To: Yuanchu Xie
  Cc: Andrew Morton, Arnd Bergmann, Arnd Bergmann, Johannes Weiner,
	Wei Xu, David Hildenbrand (Red Hat),
	Michal Hocko, Qi Zheng, Shakeel Butt, Lorenzo Stoakes,
	Baolin Wang, Kairui Song, Davidlohr Bueso, Koichiro Den,
	Jiayuan Chen, Bertrand Wlodarczyk, linux-mm, linux-kernel

On Tue, Feb 17, 2026 at 12:55 PM Yuanchu Xie <yuanchu@google.com> wrote:
>
> Hi Andrew and Arnd,
>
> On Fri, Feb 13, 2026 at 11:23 AM Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >
> > On Fri, 13 Feb 2026 18:07:04 +0100 "Arnd Bergmann" <arnd@arndb.de> wrote:
> >
> > > On Fri, Feb 13, 2026, at 17:58, Andrew Morton wrote:
> > > > On Fri, 13 Feb 2026 13:38:56 +0100 Arnd Bergmann <arnd@kernel.org> wrote:
> > > >
> > > >> From: Arnd Bergmann <arnd@arndb.de>
> > > >>
> > > >> When the -fsanitize=bounds sanitizer is enabled,
> > > >
> > > > Is this an option in current kernels?
> > >
> > > Yes, this is CONFIG_UBSAN_ARRAY_BOUNDS. The actual warning
> > > only shows up in some configurations with that, so either there
> > > is some other dependency, or an element of chance based on gcc
> > > optimizations.
> >
> > OK, I'll put a cc:stable on this, as people will want to compile older
> > kernels with gcc-16.
> >
> > Aiming for upstreaming into 7.1-rc1 unless it's more urgent than I
> > think.
> >
> > > > Seems a large change just to squish a compiler warning.  People might
> > > > prefer a simple
> > > >
> > > > -   struct ctrl_pos sp, pv;
> > > > +   struct ctrl_pos sp, pv = {};
> > >
> > > Right, that would clearly also shut up the warning.
> > >
> > > To me this seems less intuitive without an extra comment,
> > > since read_ctrl_pos() is meant to initialize the entire
> > > struct, but please pick whichever you find most readable
> > > here.
> >
> > Let's see what the MGLRU maintainers have to say.
>
> I went over the various cases of read_ctrl_pos and couldn't find
> anything wrong (let me know if I'm mistaken), so this seems like a
> mild compiler bug we're trying to work around. Given it's one single
> version of gcc, I'm a fan of the simple `struct ctrl_pos sp, pv = {};`
> with a comment.

Agreed, that simpler change with a comment seems better to me.

I don't necessarily hate the refactor to split up the single /
aggregate cases, but if we were going to do that I'd probably have
aggregate_ctrl_pos call read_ctrl_pos to avoid duplicating the code,
and I'd want to consider marking read_ctrl_pos inline.

But, it does seem like a large delta just to work around a false
positive warning. The simpler change would be easier to backport to
stable too for example. So on balance I'd prefer that.

>
> Thanks,
> Yuanchu


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-02-17 21:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-02-13 12:38 [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning Arnd Bergmann
2026-02-13 16:58 ` Andrew Morton
2026-02-13 17:07   ` Arnd Bergmann
2026-02-13 17:23     ` Andrew Morton
2026-02-17 20:55       ` Yuanchu Xie
2026-02-17 21:22         ` Axel Rasmussen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox