On Thu, Jun 27, 2024 at 04:32:03AM GMT, Yosry Ahmed wrote: > On Thu, Jun 27, 2024 at 3:33 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: [...] > > > > The reason why I suggested that the completion live in struct cgroup > > is because there is a chance here that the flush completes and another > > irrelevant flush starts between reading cgrp_rstat_ongoing_flusher and > > calling wait_for_completion_interruptible_timeout(). Yes this can happen if flusher for irrelevant cgroup calls reinit_completion() while the initial flusher was just about to call wait_for_completion_interruptible_timeout(). > > > > This will cause the caller to wait for an irrelevant flush, which may > > be fine because today the caller would wait for the lock anyway. Just > > mentioning this in case you think this may happen enough to be a > > problem. > > Actually, I think this can happen beyond the window I described above. > I think it's possible that a thread waits for the flush, then gets > woken up when complete_all() is called, but another flusher calls > reinit_completion() immediately. The woken up thread will observe > completion->done == 0 and go to sleep again. I don't think it will go to sleep again as there is no retry. > > I think most of these cases can be avoided if we make the completion > per cgroup. It is still possible to wait for more flushes than > necessary, but only if they are for the same cgroup. Yeah, per-cgroup completion would avoid the problem of waiting for irrelevant flush.
