linux-kernel - Re: [PATCH v2 1/4] mm/page_alloc/vmstat: Simplify refresh_cpu_vm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250926182438.3108364-1-joshua.hahnjy@gmail.com>
Date: Fri, 26 Sep 2025 11:24:37 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: SeongJae Park <sj@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	Chris Mason <clm@...com>,
	Kiryl Shutsemau <kirill@...temov.name>,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>,
	Brendan Jackman <jackmanb@...gle.com>,
	David Hildenbrand <david@...hat.com>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Michal Hocko <mhocko@...e.com>,
	Mike Rapoport <rppt@...nel.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Vlastimil Babka <vbabka@...e.cz>,
	Zi Yan <ziy@...dia.com>,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org,
	kernel-team@...a.com
Subject: Re: [PATCH v2 1/4] mm/page_alloc/vmstat: Simplify refresh_cpu_vm_stats change detection

Hi SJ, thank you for reviewing my patch!

> >  void page_alloc_init_cpuhp(void);
> > -int decay_pcp_high(struct zone *zone, struct per_cpu_pages *pcp);
> > +bool decay_pcp_high(struct zone *zone, struct per_cpu_pages *pcp);
> >  void drain_zone_pages(struct zone *zone, struct per_cpu_pages *pcp);
> >  void drain_all_pages(struct zone *zone);
> >  void drain_local_pages(struct zone *zone);
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index d1d037f97c5f..77e7d9a5f149 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2561,10 +2561,10 @@ static int rmqueue_bulk(struct zone *zone, unsigned int order,
> >   * Called from the vmstat counter updater to decay the PCP high.
> >   * Return whether there are addition works to do.
> >   */
> > -int decay_pcp_high(struct zone *zone, struct per_cpu_pages *pcp)
> > +bool decay_pcp_high(struct zone *zone, struct per_cpu_pages *pcp)
> >  {
> >  	int high_min, to_drain, batch;
> > -	int todo = 0;
> > +	bool todo;
> 
> I know you and others already found 'todo' should be initialized. :)

Yes, I'll be sure to fix this in the next version! : -)

> [...]
> > diff --git a/mm/vmstat.c b/mm/vmstat.c
> > index 71cd1ceba191..1f74a3517ab2 100644
> > --- a/mm/vmstat.c
> > +++ b/mm/vmstat.c
> [...]
> > @@ -839,7 +839,7 @@ static int refresh_cpu_vm_stats(bool do_pagesets)
> >  		if (do_pagesets) {
> >  			cond_resched();
> >  
> > -			changes += decay_pcp_high(zone, this_cpu_ptr(pcp));
> > +			changed |= decay_pcp_high(zone, this_cpu_ptr(pcp));
> 
> I'm not a fan of bit operations unless it provides clear benefits.
> What about below?
> 
>     if (decay_pcp_high(zone, this_cpu_ptr(pcp)) && !changed)
>     	changed = truee;

Here, what if I change it to just:

	if (decay_pcp_high(zone, this_cpu_ptr(pcp))
		changed = true;

Since even if changed == true already, this will be a no-op.

> Just a personal and trivial taste.  No strong opinion.  If you don't strongly
> feel my suggestion is better, please keep the original code.

I feel like if someone (you) feels like bitwise operations here makes it
less clear what the code is doing, others may feel the same way as well!
Happy to make the change to hopefully make it more easily understandable
what is happening. 

> >  #ifdef CONFIG_NUMA
> >  			/*
> >  			 * Deal with draining the remote pageset of this
> > @@ -861,13 +861,13 @@ static int refresh_cpu_vm_stats(bool do_pagesets)
> >  			}
> >  
> >  			if (__this_cpu_dec_return(pcp->expire)) {
> > -				changes++;
> > +				changed = true;
> >  				continue;
> >  			}
> >  
> >  			if (__this_cpu_read(pcp->count)) {
> >  				drain_zone_pages(zone, this_cpu_ptr(pcp));
> > -				changes++;
> > +				changed = true;
> >  			}
> >  #endif
> >  		}
> > @@ -887,8 +887,8 @@ static int refresh_cpu_vm_stats(bool do_pagesets)
> >  		}
> >  	}
> >  
> > -	changes += fold_diff(global_zone_diff, global_node_diff);
> > -	return changes;
> > +	changed |= fold_diff(global_zone_diff, global_node_diff);
> > +	return changed;
> 
> Ditto.

Will make the same change here!

> >  }
> >  
> >  /*
> > -- 
> > 2.47.3
> 
> Other than the above trivial things, all looks good to me :)

Thanks SJ, I hope you have a great day!
Joshua

> Thanks,
> SJ