Re: [PATCH v1 3/4] mm: introduce page memcg flags

From: Johannes Weiner <hannes@cmpxchg.org>
To: Roman Gushchin <guro@fb.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Shakeel Butt <shakeelb@google.com>,
	Michal Hocko <mhocko@kernel.org>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	kernel-team@fb.com
Subject: Re: [PATCH v1 3/4] mm: introduce page memcg flags
Date: Thu, 24 Sep 2020 16:01:22 -0400	[thread overview]
Message-ID: <20200924200122.GC329853@cmpxchg.org> (raw)
In-Reply-To: <20200922203700.2879671-4-guro@fb.com>

On Tue, Sep 22, 2020 at 01:36:59PM -0700, Roman Gushchin wrote:
> The lowest bit in page->memcg_data is used to distinguish between
> struct memory_cgroup pointer and a pointer to a objcgs array.
> All checks and modifications of this bit are open-coded.
> 
> Let's formalize it using page memcg flags, defined in page_memcg_flags
> enum and replace all open-coded accesses with test_bit()/__set_bit().
> 
> Few additional flags might be added later. Flags are intended to be
> mutually exclusive.
> 
> Signed-off-by: Roman Gushchin <guro@fb.com>
> ---
>  include/linux/memcontrol.h | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index ab3ea3e90583..9a49f1e1c0c7 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -343,6 +343,11 @@ struct mem_cgroup {
>  
>  extern struct mem_cgroup *root_mem_cgroup;
>  
> +enum page_memcg_flags {
> +	/* page->memcg_data is a pointer to an objcgs vector */
> +	PG_MEMCG_OBJ_CGROUPS,

How about enum memcg_data_flags and PGMEMCG_OBJCG?

> @@ -371,13 +376,7 @@ static inline struct mem_cgroup *page_mem_cgroup_check(struct page *page)
>  {
>  	unsigned long memcg_data = page->memcg_data;
>  
> -	/*
> -	 * The lowest bit set means that memcg isn't a valid
> -	 * memcg pointer, but a obj_cgroups pointer.
> -	 * In this case the page is shared and doesn't belong
> -	 * to any specific memory cgroup.
> -	 */
> -	if (memcg_data & 0x1UL)
> +	if (test_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data))
>  		return NULL;
>  
>  	return (struct mem_cgroup *)memcg_data;
> @@ -422,7 +421,13 @@ static inline void clear_page_mem_cgroup(struct page *page)
>   */
>  static inline struct obj_cgroup **page_obj_cgroups(struct page *page)
>  {
> -	return (struct obj_cgroup **)(page->memcg_data & ~0x1UL);
> +	unsigned long memcg_data = page->memcg_data;
> +
> +	VM_BUG_ON_PAGE(memcg_data && !test_bit(PG_MEMCG_OBJ_CGROUPS,
> +					       &memcg_data), page);
> +	__clear_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data);

The flag names make sense to me, but this shouldn't be using test_bit,
__clear_bit, __set_bit etc. on local variables. It suggests that it's
modifying some shared/global state, when it's just masking out a bit
during a read. We usually just open-code the bitwise ops for that.