On Fri, Aug 22, 2025 at 2:00 AM Julian Sun <sunjunchao@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Fri, Aug 22, 2025 at 12:59 AM Tejun Heo <tj@xxxxxxxxxx> wrote:
> >
> > 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.
> >
>
> Do you mean logic like this?
>
> for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++)
> wb_wait_for_completion(&memcg->cgwb_frn[i].done);
> kfree(memcg);
>
> But there still exist task hang issues as long as
> wb_wait_for_completion() exists.
> I think the scope of impact of the current changes should be
> manageable. I have checked all the other places where wb_queue_work()
> is called, and their free_done values are all 0, and I also tested
> this patch with the reproducer in [1] with kasan and kmemleak enabled.
> The test result looks fine, so this should not have a significant
> impact.
BTW, the test case is like this — it ran for over a night.
while true; do ./repro.sh && sleep 300 && ./stop.sh; done
sjc@debian:~/linux$ cat stop.sh
#!/bin/bash
#
TEST=/sys/fs/cgroup/test
A=$TEST/A
B=$TEST/B
echo "-memory" > $TEST/cgroup.subtree_control
pkill write-range
sync
sleep 5
sync
sleep 5
echo 3 > /proc/sys/vm/drop_caches
rmdir $A $B
> What do you think?
>
> [1]: https://lore.kernel.org/all/20190821210235.GN2263813@xxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > Thanks.
> >
> > --
> > tejun
>
>
> Thanks,
> --
> Julian Sun <sunjunchao@xxxxxxxxxxxxx>
Thanks,
--
Julian Sun <sunjunchao@xxxxxxxxxxxxx>
