On Tue, Aug 22, 2023 at 9:35 AM Michal Koutný <mkoutny@xxxxxxxx> wrote: > > On Tue, Aug 22, 2023 at 09:00:06AM -0700, Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > We can probably also kick FLUSH_TIME forward as well. > > True, they guard same work. > > > Perhaps I can move both into do_stats_flush() then. If I understand > > correctly this is what you mean? > > Yes. > > > What do you think? > > The latter is certainly better looking code. > > I wasn't sure at first about moving stats_flush_threshold reset before > actual flush but on second thought it should not be a significant change > - readers: may skip flushing, the values that they read should still be > below the error threshold, Unified readers will skip anyway as there's an ongoing flush, non-unified readers don't check the threshold anyway (with this patch series). So for readers it should not be a change. > - writers: may be slowed down a bit (because of conditional atomic write > optimization in memcg_rstat_updates), presumably not on average > though. Yeah writers will start doing atomic writes once a flush starts instead of once it ends. I don't think it will matter in practice though. The optimization is only effective if we manage to surpass the threshold before the periodic flusher (or any unified flusher) kicks in and resets it. If we start doing atomic writes earlier, then we will also stop earlier; the number of atomic writes should stay the same. I think the only difference will be the scenario where we start atomic writes early but the periodic flush happens before we reach the threshold, in which case we aren't doing a lot of updates anyway. I hope this makes _any_ sense :) > > So the latter should work too. > I will include that in v2. I will wait for a bit of further review comments on this version first. Thanks for taking a look! > Michal
