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

[Julian Sun <sunjunchao@bytedance.com>]

Hi,

On Thu, Aug 28, 2025 at 12:25 AM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Tue, Aug 26, 2025 at 08:16:18PM +0800, Julian Sun wrote:
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 785173aa0739..f6dd771df369 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_wq_entry {
> > +     struct wb_completion *done;
> > +     struct wait_queue_entry wq_entry;
> > +};
>
> Why not embed wb_completion in the sturct? Also, can you name it
> cgwb_frn_wait instead?

Sure, will update it in next version.
>
> >  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_wq_entry *frn_wq; /* used to free resources when release memcg */
>
> And the field just "wait". I know wq is used as an abbreviation for waitq
> but it conflicts with workqueue and waitq / wait names seem clearer.

Reasonable. Will update it in next version.
>
> > +static int memcg_cgwb_waitq_callback_fn(struct wait_queue_entry *wq_entry, unsigned int mode,
> > +                                     int flags, void *key)
> > +{
> > +     struct cgwb_frn_wq_entry *frn_wq_entry = container_of(wq_entry,
> > +                                                     struct cgwb_frn_wq_entry, wq_entry);
> > +
> > +     list_del_init_careful(&wq_entry->entry);
>
> Why list_del_init_careful() instead of just list_del()?
>
> > +     kfree(frn_wq_entry->done);
> > +     kfree(frn_wq_entry);
>
> If done is embedded, this will become one free, right?

Yes, this makes it look clearer. Thank you for your review.
>
> Thanks.
>
> --
> tejun


Thanks,
-- 
Julian Sun <sunjunchao@bytedance.com>