Re: [PATCH v3] memcg: Don't wait writeback completion when release memcg.

[Tejun Heo <tj@kernel.org>]

Hello,

On Thu, Aug 28, 2025 at 02:13:56AM +0800, Julian Sun wrote:
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 2ad261082bba..6c1ed286da6a 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -65,6 +65,13 @@ struct wb_completion {
>  	wait_queue_head_t	*waitq;
>  };
>  
> +static inline void wb_completion_init(struct wb_completion *done,
> +									  struct wait_queue_head *waitq)

Indentation.

> +{
> +	atomic_set(&done->cnt, 1);
> +	done->waitq = waitq;
> +}
> +
>  #define __WB_COMPLETION_INIT(_waitq)	\
>  	(struct wb_completion){ .cnt = ATOMIC_INIT(1), .waitq = (_waitq) }
>  
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 785173aa0739..24e881ce4909 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -157,11 +157,17 @@ struct mem_cgroup_thresholds {
>   */
>  #define MEMCG_CGWB_FRN_CNT	4
>  
> +struct cgwb_frn_wait {
> +	struct wb_completion done;
> +	struct wait_queue_entry wq_entry;
> +};
> +
>  struct memcg_cgwb_frn {
>  	u64 bdi_id;			/* bdi->id of the foreign inode */
>  	int memcg_id;			/* memcg->css.id of foreign inode */
>  	u64 at;				/* jiffies_64 at the time of dirtying */
> -	struct wb_completion done;	/* tracks in-flight foreign writebacks */
> +	struct wb_completion *done;	/* tracks in-flight foreign writebacks */
> +	struct cgwb_frn_wait *wait;	/* used to free resources when release memcg */

Is ->done still needed? Can't it just do frn->wait.done?

> +#ifdef CONFIG_CGROUP_WRITEBACK
> +static int memcg_cgwb_waitq_callback_fn(struct wait_queue_entry *wq_entry, unsigned int mode,
> +					int flags, void *key)
> +{
> +	struct cgwb_frn_wait *frn_wait = container_of(wq_entry,
> +						      struct cgwb_frn_wait, wq_entry);
> +
> +	list_del(&wq_entry->entry);
> +	kfree(frn_wait);
> +
> +	return 0;
> +}
> +#endif

Note that the above will be called for all queued waits when any one entry
triggers. It'd need to check whether done is zero before self-deleting and
freeing.

> @@ -3912,8 +3938,18 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css)
>  	int __maybe_unused i;
>  
>  #ifdef CONFIG_CGROUP_WRITEBACK
> -	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++)
> -		wb_wait_for_completion(&memcg->cgwb_frn[i].done);
> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
> +		struct memcg_cgwb_frn *frn = &memcg->cgwb_frn[i];
> +
> +		if (atomic_dec_and_test(&frn->done->cnt))
> +			kfree(frn->wait);
> +		else
> +			/*
> +			 * Not necessary to wait for wb completion which might cause task hung,
> +			 * only used to free resources. See memcg_cgwb_waitq_callback_fn().
> +			 */
> +			__add_wait_queue_entry_tail(frn->done->waitq, &frn->wait->wq_entry);
> +	}

And then, this can probably be simplified to sth like:

        __add_wait_queue_entry_tail(...);
        if (atomic_dec_and_test(&frn->done->dnt);
                wake_up_all(waitq);

Thanks.

-- 
tejun