在 2025/8/25 18:13, Jan Kara 写道:
Hi, Jan
Thanks for your review and comments.
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...
AFAICT the mechanism is used to make the max number of wb works that we
issue concurrently less than MEMCG_CGWB_FRN_CNT(4). If we replace the
oldest and flush wb work whether the writeback is running or not, it
seems that we are likely to flush more than MEMCG_CGWB_FRN_CNT wb works
concurrently, I'm not sure how much influence this will have...
Honza
Thanks,
--
Julian Sun <sunjunchao@xxxxxxxxxxxxx>
