Re: [PATCH for-next-fixes v2 1/5] mm/memcontrol: defer obj_cgroup_release() to irq_work

[Vlastimil Babka (SUSE)]

On 11/3/26 15:47, Harry Yoo (Meta) wrote:
> obj_cgroup_put() can be called from any context.
> __memcg_slab_free_hook() puts a per-object reference on
> every accounted free, and draining a per-cpu objcg stock slot puts
> the cached reference. When it drops the last reference,
> obj_cgroup_release() takes objcg_lock, frees percpu memory
> (pcpu_lock, via percpu_ref_exit()) and calls kfree_rcu(). Each of
> these can deadlock when the current context interrupted a holder of
> the respective lock.
> 
> Defer the release to irq_work. llist_add() and irq_work_queue() are
> NMI-safe, and the irq_work runs in a context where acquiring a
> spinlock is allowed.
> 
> Add a separate llist_node instead of reusing the list/rcu union
> because the list stays linked until the deferred release. struct
> obj_cgroup grows from 56 to 64 bytes, but both are served from
> kmalloc-64, so no memory is additionally consumed.
> 
> Closes: https://lore.kernel.org/20261001044056.75079-3-kmehltretter@xxxxxxxxx
> Fixes: af92793e52c3 ("slab: Introduce kmalloc_nolock() and kfree_nolock().")
> Signed-off-by: Harry Yoo (Meta) <harry@xxxxxxxxxx>

I guess releasing objcg isn't too frequent, so it won't matter that we make
all cases use the defered work instead of somehow trying to do that only in
contexts that would really need it.

Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>

since memcg tree will be used from the next cycle, we can just keep this as
part of the series in the slab tree?

> ---
>  include/linux/memcontrol.h |  2 ++
>  mm/memcontrol.c            | 27 ++++++++++++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 69e1bcc67c58..4d5c634daef8 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -185,6 +185,8 @@ struct obj_cgroup {
>  		struct list_head list; /* protected by objcg_lock */
>  		struct rcu_head rcu;
>  	};
> +	/* Used to defer obj_cgroup_release() to irq_work */
> +	struct llist_node release_node;
>  	bool is_root;
>  };
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a05d9a8baa1b..c53147a88580 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -147,9 +147,8 @@ static void memcg_uncharge_kmem(struct mem_cgroup *memcg, unsigned int nr_pages)
>  		memcg_uncharge(memcg, nr_pages);
>  }
>  
> -static void obj_cgroup_release(struct percpu_ref *ref)
> +static void __obj_cgroup_release(struct obj_cgroup *objcg)
>  {
> -	struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt);
>  	unsigned int nr_bytes;
>  	unsigned int nr_pages;
>  	unsigned long flags;
> @@ -190,10 +189,32 @@ static void obj_cgroup_release(struct percpu_ref *ref)
>  	list_del(&objcg->list);
>  	spin_unlock_irqrestore(&objcg_lock, flags);
>  
> -	percpu_ref_exit(ref);
> +	percpu_ref_exit(&objcg->refcnt);
>  	kfree_rcu(objcg, rcu);
>  }
>  
> +static LLIST_HEAD(objcg_release_llist);
> +
> +static void objcg_release_workfn(struct irq_work *work)
> +{
> +	struct llist_node *head = llist_del_all(&objcg_release_llist);
> +	struct obj_cgroup *objcg, *tmp;
> +
> +	llist_for_each_entry_safe(objcg, tmp, head, release_node)
> +		__obj_cgroup_release(objcg);
> +}
> +
> +static DEFINE_IRQ_WORK(objcg_release_work, objcg_release_workfn);
> +
> +static void obj_cgroup_release(struct percpu_ref *ref)
> +{
> +	struct obj_cgroup *objcg = container_of(ref, struct obj_cgroup, refcnt);
> +
> +	/* The last put may come from a context that cannot take spinlocks */
> +	if (llist_add(&objcg->release_node, &objcg_release_llist))
> +		irq_work_queue(&objcg_release_work);
> +}
> +
>  static struct obj_cgroup *obj_cgroup_alloc(void)
>  {
>  	struct obj_cgroup *objcg;
>