Re: [PATCH v2] mm: kmem: add lockdep assertion to obj_cgroup_memcg

[Shakeel Butt]

On Wed, Jul 24, 2024 at 05:20:09PM GMT, Vlastimil Babka (SUSE) wrote:
> On 7/24/24 11:53 AM, 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.
> > 
> > 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).
> > Additionally, this is some internal implementation detail of memcg
> > and should not be accessible outside memcg code.
> > 
> > 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@xxxxxxxxxxxxx/
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > ---
> > v2:
> >  - Remove mention of objcg_lock in obj_cgroup_memcg()(Shakeel Butt).
> > 
> >  include/linux/memcontrol.h | 20 +++++++++++++++++---
> >  mm/memcontrol.c            |  6 +++---
> >  2 files changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index fc94879db4dff..742351945f683 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -360,11 +360,11 @@ 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(&cgroup_mutex));
> 
> Maybe lockdep_assert_once() would be a better fit?
> 

So something like:
	lockdep_assert_once(rcu_read_lock_held() || lockdep_is_held(&cgroup_mutex));