Message-ID: <d30c7de3-65b5-4b5c-9046-4eed2c7d0b57@linux.dev>
Date: Mon, 22 Jul 2024 15:46:02 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Muchun Song <songmuchun@...edance.com>, hannes@...xchg.org,
 mhocko@...nel.org, roman.gushchin@...ux.dev, shakeel.butt@...ux.dev,
 muchun.song@...ux.dev, akpm@...ux-foundation.org
Cc: cgroups@...r.kernel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: kmem: add lockdep assertion to obj_cgroup_memcg

On 2024/7/22 15:08, Muchun Song wrote:
> The obj_cgroup_memcg() is supposed to safe to prevent the returned
> memory cgroup from being freed only when the caller is holding the
> rcu read lock or objcg_lock or cgroup_mutex. It is very easy to
> ignore thoes conditions when users call some upper APIs which call
> obj_cgroup_memcg() internally like mem_cgroup_from_slab_obj() (See
> the link below). So it is better to add lockdep assertion to
> obj_cgroup_memcg() to find those issues ASAP.

Yeah, some users care about the lifetime of returned memcg, while
some other users maybe not.

Maybe a dumb question, can we just make objcg hold the refcount of
its pointed memcg? So the users of that objcg don't need to care
about the refcount of memcg? (We could switch the refcount from
old memcg to the new memcg when objcg switch memcg pointer, right?)

Thanks.

> 
> Because there is no user of obj_cgroup_memcg() holding objcg_lock
> to make the returned memory cgroup safe, do not add objcg_lock
> assertion (We should export objcg_lock if we really want to do)
> and leave a comment to indicate it is intentional.
> 
> Some users like __mem_cgroup_uncharge() do not care the lifetime
> of the returned memory cgroup, which just want to know if the
> folio is charged to a memory cgroup, therefore, they do not need
> to hold the needed locks. In which case, introduce a new helper
> folio_memcg_charged() to do this. Compare it to folio_memcg(), it
> could eliminate a memory access of objcg->memcg for kmem, actually,
> a really small gain.
> 
> Link: https://lore.kernel.org/all/20240718083607.42068-1-songmuchun@bytedance.com/
> Signed-off-by: Muchun Song <songmuchun@...edance.com>
> ---
>   include/linux/memcontrol.h | 22 +++++++++++++++++++---
>   mm/memcontrol.c            |  6 +++---
>   2 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index fc94879db4dff..d616c50025098 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -360,11 +360,13 @@ static inline bool folio_memcg_kmem(struct folio *folio);
>    * After the initialization objcg->memcg is always pointing at
>    * a valid memcg, but can be atomically swapped to the parent memcg.
>    *
> - * The caller must ensure that the returned memcg won't be released:
> - * e.g. acquire the rcu_read_lock or css_set_lock.
> + * The caller must ensure that the returned memcg won't be released.
>    */
>   static inline struct mem_cgroup *obj_cgroup_memcg(struct obj_cgroup *objcg)
>   {
> +	WARN_ON_ONCE(!rcu_read_lock_held() &&
> +		  /* !lockdep_is_held(&objcg_lock) && */
> +		     !lockdep_is_held(&cgroup_mutex));
>   	return READ_ONCE(objcg->memcg);
>   }
>   
> @@ -438,6 +440,19 @@ static inline struct mem_cgroup *folio_memcg(struct folio *folio)
>   	return __folio_memcg(folio);
>   }
>   
> +/*
> + * folio_memcg_charged - If a folio is charged to a memory cgroup.
> + * @folio: Pointer to the folio.
> + *
> + * Returns true if folio is charged to a memory cgroup, otherwise returns false.
> + */
> +static inline bool folio_memcg_charged(struct folio *folio)
> +{
> +	if (folio_memcg_kmem(folio))
> +		return __folio_objcg(folio) != NULL;
> +	return __folio_memcg(folio) != NULL;
> +}
> +
>   /**
>    * folio_memcg_rcu - Locklessly get the memory cgroup associated with a folio.
>    * @folio: Pointer to the folio.
> @@ -454,7 +469,6 @@ static inline struct mem_cgroup *folio_memcg_rcu(struct folio *folio)
>   	unsigned long memcg_data = READ_ONCE(folio->memcg_data);
>   
>   	VM_BUG_ON_FOLIO(folio_test_slab(folio), folio);
> -	WARN_ON_ONCE(!rcu_read_lock_held());
>   
>   	if (memcg_data & MEMCG_DATA_KMEM) {
>   		struct obj_cgroup *objcg;
> @@ -463,6 +477,8 @@ static inline struct mem_cgroup *folio_memcg_rcu(struct folio *folio)
>   		return obj_cgroup_memcg(objcg);
>   	}
>   
> +	WARN_ON_ONCE(!rcu_read_lock_held());
> +
>   	return (struct mem_cgroup *)(memcg_data & ~OBJEXTS_FLAGS_MASK);
>   }
>   
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 622d4544edd24..3da0284573857 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2366,7 +2366,7 @@ void mem_cgroup_cancel_charge(struct mem_cgroup *memcg, unsigned int nr_pages)
>   
>   static void commit_charge(struct folio *folio, struct mem_cgroup *memcg)
>   {
> -	VM_BUG_ON_FOLIO(folio_memcg(folio), folio);
> +	VM_BUG_ON_FOLIO(folio_memcg_charged(folio), folio);
>   	/*
>   	 * Any of the following ensures page's memcg stability:
>   	 *
> @@ -4617,7 +4617,7 @@ void __mem_cgroup_uncharge(struct folio *folio)
>   	struct uncharge_gather ug;
>   
>   	/* Don't touch folio->lru of any random page, pre-check: */
> -	if (!folio_memcg(folio))
> +	if (!folio_memcg_charged(folio))
>   		return;
>   
>   	uncharge_gather_clear(&ug);
> @@ -4662,7 +4662,7 @@ void mem_cgroup_replace_folio(struct folio *old, struct folio *new)
>   		return;
>   
>   	/* Page cache replacement: new folio already charged? */
> -	if (folio_memcg(new))
> +	if (folio_memcg_charged(new))
>   		return;
>   
>   	memcg = folio_memcg(old);

Powered by blists - more mailing lists