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 for Android: free password hash cracker in your pocket

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

Message-Id: <20250926175004.54610-1-sj@kernel.org>
Date: Fri, 26 Sep 2025 10:50:04 -0700
From: SeongJae Park <sj@...nel.org>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
	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

On Wed, 24 Sep 2025 13:44:05 -0700 Joshua Hahn <joshua.hahnjy@...il.com> wrote:

> Currently, refresh_cpu_vm_stats returns an int, indicating how many
> changes were made during its updates. Using this information, callers
> like vmstat_update can heuristically determine if more work will be done
> in the future.
> 
> However, all of refresh_cpu_vm_stats's callers either (a) ignore the
> result, only caring about performing the updates, or (b) only care about
> whether changes were made, but not *how many* changes were made.
> 
> Simplify the code by returning a bool instead to indicate if updates
> were made.
> 
> In addition, simplify fold_diff and decay_pcp_high to return a bool
> for the same reason.
> 
> Signed-off-by: Joshua Hahn <joshua.hahnjy@...il.com>
> ---
>  include/linux/gfp.h |  2 +-
>  mm/page_alloc.c     |  8 ++++----
>  mm/vmstat.c         | 26 +++++++++++++-------------
>  3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 5ebf26fcdcfa..63c72cb1d117 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -386,7 +386,7 @@ extern void free_pages(unsigned long addr, unsigned int order);
>  #define free_page(addr) free_pages((addr), 0)
>  
>  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. :)

[...]
> 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;

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

>  #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.

>  }
>  
>  /*
> -- 
> 2.47.3

Other than the above trivial things, all looks good to me :)


Thanks,
SJ