Re: [PATCH 03/11] mm: memcontrol: constify the lruvec helpers

[Tal Zussman]

On 9/2/26 10:27 PM, Tal Zussman wrote:
> The lruvec lookup helpers only read from the memcg, folio, or lruvec
> they are given. Constify them, along with lruvec_pgdat().
> 
> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
> ---
>  include/linux/memcontrol.h | 34 +++++++++++++++++-----------------
>  include/linux/mmzone.h     |  2 +-
>  mm/memcontrol.c            |  6 +++---
>  3 files changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 2b3b62e84ab2..4b7cc97e45b2 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -688,7 +688,7 @@ void mem_cgroup_migrate(struct folio *old, struct folio *new);
>   * @pgdat combination. This can be the node lruvec, if the memory
>   * controller is disabled.
>   */
> -static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
> +static inline struct lruvec *mem_cgroup_lruvec(const struct mem_cgroup *memcg,
>  					       struct pglist_data *pgdat)
>  {
>  	struct mem_cgroup_per_node *mz;

>From Sashiko:

[Severity: Medium]
Does this function actually just read from the memcg?

Looking at the rest of mem_cgroup_lruvec() not shown in this diff, it
contains:

     if (unlikely(lruvec->pgdat != pgdat))
         lruvec->pgdat = pgdat;

This mutates the lruvec which is logically part of the memcg's per-node
state.

Since the commit message states these helpers "only read from the memcg",
is it safe to constify this when it can modify the structure on newly
onlined nodes?

Answer:

The write goes to lruvec->pgdat, which is stored in the per-node
structure reached through memcg->nodeinfo[], not in the memcg itself,
so const is fine. I'll clarify the commit message.

> @@ -1528,8 +1528,8 @@ static inline void lruvec_unlock_irqrestore(struct lruvec *lruvec, unsigned long
>  }
>  
>  /* Test requires a stable folio->memcg binding, see folio_memcg() */
> -static inline bool folio_matches_lruvec(struct folio *folio,
> -		struct lruvec *lruvec)
> +static inline bool folio_matches_lruvec(const struct folio *folio,
> +		const struct lruvec *lruvec)
>  {
>  	return lruvec_pgdat(lruvec) == folio_pgdat(folio) &&
>  	       lruvec_memcg(lruvec) == folio_memcg(folio);

>From Sashiko:

[Severity: Low]
Were folio_lruvec_relock_irq() and folio_lruvec_relock_irqsave()
intentionally omitted from the constification?

Just below this hunk in include/linux/memcontrol.h, both functions
still take a non-const struct folio pointer:

static inline struct lruvec *folio_lruvec_relock_irq(struct folio *folio,
         struct lruvec *locked_lruvec)

static inline void folio_lruvec_relock_irqsave(struct folio *folio,
         struct lruvec **lruvecp, unsigned long *flags)

Since they only pass the folio along to other now-constified helpers,
should they be updated to maintain API consistency?

Answer:

Will constify their folio as well.