Hello,
On Thu, Aug 21, 2025 at 10:30:30AM +0800, Julian Sun wrote:
> On Thu, Aug 21, 2025 at 4:58 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
> >
> > On Wed, Aug 20, 2025 at 07:19:40PM +0800, Julian Sun wrote:
> > > @@ -3912,8 +3921,12 @@ 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 wb_completion *done = memcg->cgwb_frn[i].done;
> > > +
> > > + if (atomic_dec_and_test(&done->cnt))
> > > + kfree(done);
> > > + }
> > > #endif
> >
> > Can't you just remove done? I don't think it's doing anything after your
> > changes anyway.
>
> Thanks for your review.
>
> AFAICT done is also used to track free slots in
> mem_cgroup_track_foreign_dirty_slowpath() and
> mem_cgroup_flush_foreign(), otherwise we have no method to know which
> one is free and might flush more than what MEMCG_CGWB_FRN_CNT allow.
>
> Am I missing something?
No, I missed that. I don't think we need to add extra mechanisms in wb for
this tho. How about shifting wb_wait_for_completion() and kfree(memcg) into
a separate function and punt those to a separate work item? That's going to
be a small self-contained change in memcg.
Thanks.
--
tejun
