[PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning

Arnd Bergmann posted 1 patch 2 months ago
There is a newer version of this series
mm/vmscan.c | 37 +++++++++++++++++++++++++------------
1 file changed, 25 insertions(+), 12 deletions(-)
[PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Arnd Bergmann 2 months ago
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
Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Andrew Morton 2 months ago
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 = {};

?
Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Arnd Bergmann 2 months ago
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
Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Andrew Morton 2 months ago
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.
Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Yuanchu Xie 1 month, 4 weeks ago
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
Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Axel Rasmussen 1 month, 4 weeks ago
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
Re: [PATCH] mm/vmscan: avoid false-positive -Wuninitialized warning
Posted by Andrew Morton 3 weeks ago
On Tue, 17 Feb 2026 13:22:01 -0800 Axel Rasmussen <axelrasmussen@google.com> wrote:

> > > 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.   Did anyone send a v2?  If so, I misplaced it!