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

[Julian Sun <sunjunchao@bytedance.com>]

Hi,

On Thu, Aug 28, 2025 at 2:58 AM Tejun Heo <tj@kernel.org> wrote:
>
> 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.

Ah, yeah, it does need to.  Have sent v4 to fix it, thanks :)
>
> > @@ -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



-- 
Julian Sun <sunjunchao@bytedance.com>