Re: [PATCH] memcg: keep propagating stats updates to ancestors

[Mingyu He]

Hi Shakeel,

  This came from a reproducible workload with a custom in-kernel consumer,
  not a conjecture or an AI analysis. I uses aggregated file pages statistics to
  decide whether reclamation is needed.

  Tracing showed leaf stats_updates at 6656 while root stayed at 6144
  (the threshold). Further updates stopped at the leaf, and root flush
  requests were skipped.
  In one reclamation check, memcg_page_state(leaf, NR_FILE_PAGES) returned
  about 2.64 GiB from the aggregated statistics. In the same invocation,
  a bpftrace probe sequentially summed the leaf's per-CPU NR_FILE_PAGES
  counters and obtained about 6.58 GiB. This sum was not an atomic snapshot,
  but it exposed a large discrepancy with the value used by the check.

  The periodic flush does restore progress. But issue for this consumer is
  the missed reclamation decisions before that flush, not permanently lost
  accounting.

  Regarding the BPF interface, the implementation in the tree I found is:

  __bpf_kfunc void bpf_mem_cgroup_flush_stats(struct mem_cgroup *memcg)
  {
          mem_cgroup_flush_stats(memcg);
  }
  This bpf function still uses memcg_rstat_updated() benethe.
  It doesn't handle the issue.

  I originally called mem_cgroup_flush_stats() before reading the counters.
  After observing this issue, I switched the consumer to css_rstat_flush()
  on the relevant subtree.

  If waiting for the 2 seconds periodic flush is acceptable for this
function even
  under continued updates, I agree there is no need to change it.

  However, the comment in memcg_rstat_updated() states:

          If @memcg is already flushable then all its ancestors are
          flushable as well

  Concurrent counter resets can invalidate that assumption, as the trace
  shows. If the early exit is retained as an acceptable trade-off, could
  we update the comment to acknowledge this and the reliance on a later
  flush to restore progress?

  Thanks,
  Runli