Re: [PATCH 08/11] mm: memcontrol: constify the memcg and lruvec stat readers

[Tal Zussman]

Responses to Sashiko below.

On 9/2/26 10:27 PM, Tal Zussman wrote:
> memcg_page_state(), memcg_events(), and the lruvec_page_state() family
> only read counters. Constify them.
> 
> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
> ---
>  include/linux/memcontrol.h | 23 ++++++++++++-----------
>  mm/memcontrol.c            | 12 ++++++------
>  2 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index bd82628d0d72e..10a197886f316 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
[ ... ]
> @@ -1392,19 +1393,19 @@ static inline bool memcg_vm_event_item_valid(enum vm_event_item idx)
>  	return false;
>  }
>  
> -static inline unsigned long lruvec_page_state(struct lruvec *lruvec,
> +static inline unsigned long lruvec_page_state(const struct lruvec *lruvec,
>  					      enum node_stat_item idx)
>  {
>  	return node_page_state(lruvec_pgdat(lruvec), idx);
>  }

[Severity: Low]
Does passing the newly constified lruvec pointer to lruvec_pgdat() discard
the const qualifier?

Looking at lruvec_pgdat() in include/linux/mmzone.h, it does this:

	return container_of(lruvec, struct pglist_data, __lruvec);

This returns a mutable container from a const pointer, circumventing the
internal const-correctness the patch intends to provide. Should
container_of_const() be used here instead?

Answer:

lruvec_pgdat() purposefully returns a non-const pgdat, as some callers
still need one at the moment (node_page_state()).

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 35f924c9f2a31..3d3eadddc1307 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -489,7 +489,7 @@ struct lruvec_stats {
>  	long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
>  };
>  
> -unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
> +unsigned long lruvec_page_state(const struct lruvec *lruvec, enum node_stat_item idx)
>  {
>  	struct mem_cgroup_per_node *pn;
>  	long x;

[Severity: Low]
Does the use of container_of() later in this function silently cast away the
const qualifier added to the lruvec parameter?

In lruvec_page_state():

	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);

This macro expansion discards the const qualifier, undermining the compiler's
ability to enforce const-correctness. Could container_of_const() be used here
instead, with pn declared as a const struct mem_cgroup_per_node *?

Answer:

Yes, this can use container_of_const(). Will change.

[ ... ]
> @@ -530,7 +530,7 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
>   * monotonically-incremented event counters are stored in
>   * enum node_stat_item.
>   */
> -unsigned long lruvec_page_state_monotonic(struct lruvec *lruvec,
> +unsigned long lruvec_page_state_monotonic(const struct lruvec *lruvec,
>  					  enum node_stat_item idx)
>  {
>  	struct mem_cgroup_per_node *pn;

[Severity: Low]
Does this function also silently lose the const qualifier through
container_of(), similar to lruvec_page_state()?

In lruvec_page_state_monotonic():

	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);

Would it be better to use container_of_const() and a const pointer for pn
to preserve the safety guarantees of this patch?

Answer:

Yes, this can use container_of_const(). Will change.

> @@ -547,7 +547,7 @@ unsigned long lruvec_page_state_monotonic(struct lruvec *lruvec,
>  	return (unsigned long)READ_ONCE(pn->lruvec_stats->state[i]);
>  }
>  
> -unsigned long lruvec_page_state_local(struct lruvec *lruvec,
> +unsigned long lruvec_page_state_local(const struct lruvec *lruvec,
>  				      enum node_stat_item idx)
>  {
>  	struct mem_cgroup_per_node *pn;

[Severity: Low]
Like the other functions above, does container_of() discard the const
qualifier here?

In lruvec_page_state_local():

	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);

Could this be updated to use container_of_const() as well to maintain strict
const-correctness?

Answer:

Yes, this can use container_of_const(). Will change.