On Thu 21-08-25 10:30:30, 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?
True, but is that mechanism really needed? Given the approximate nature of
foreign flushing, couldn't we just always replace the oldest foreign entry
regardless of whether the writeback is running or not? I didn't give too
deep thought to this but from a quick look this should work just fine...
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
