On Thu, Feb 02, 2023 at 04:17:18PM -0800, Yosry Ahmed wrote: > On Thu, Feb 2, 2023 at 4:01 PM Dave Chinner <david@xxxxxxxxxxxxx> wrote: > > > Patch 1 is just refactoring updating reclaim_state into a helper > > > function, and renames reclaimed_slab to just reclaimed, with a comment > > > describing its true purpose. > > > > > > Patch 2 ignores pages reclaimed outside of LRU reclaim in memcg reclaim. > > > > > > The original draft was a little bit different. It also kept track of > > > uncharged objcg pages, and reported them only in memcg reclaim and only > > > if the uncharged memcg is in the subtree of the memcg under reclaim. > > > This was an attempt to make reporting of memcg reclaim even more > > > accurate, but was dropped due to questionable complexity vs benefit > > > tradeoff. It can be revived if there is interest. > > > > > > Yosry Ahmed (2): > > > mm: vmscan: refactor updating reclaimed pages in reclaim_state > > > mm: vmscan: ignore non-LRU-based reclaim in memcg reclaim > > > > > > fs/inode.c | 3 +-- > > > > Inodes and inode mapping pages are directly charged to the memcg > > that allocated them and the shrinker is correctly marked as > > SHRINKER_MEMCG_AWARE. Freeing the pages attached to the inode will > > account them correctly to the related memcg, regardless of which > > memcg is triggering the reclaim. Hence I'm not sure that skipping > > the accounting of the reclaimed memory is even correct in this case; > > Please note that we are not skipping any accounting here. The pages > are still uncharged from the memcgs they are charged to (the allocator > memcgs as you pointed out). We just do not report them in the return > value of try_to_free_mem_cgroup_pages(), to avoid over-reporting. I was wondering the same thing as Dave, reading through this. But you're right, we'll catch the accounting during uncharge. Can you please add a comment on the !cgroup_reclaim() explaining this? There is one wrinkle with this, though. We have the following (simplified) sequence during charging: nr_reclaimed = try_to_free_mem_cgroup_pages(mem_over_limit, nr_pages, gfp_mask, reclaim_options); if (mem_cgroup_margin(mem_over_limit) >= nr_pages) goto retry; /* * Even though the limit is exceeded at this point, reclaim * may have been able to free some pages. Retry the charge * before killing the task. * * Only for regular pages, though: huge pages are rather * unlikely to succeed so close to the limit, and we fall back * to regular pages anyway in case of failure. */ if (nr_reclaimed && nr_pages <= (1 << PAGE_ALLOC_COSTLY_ORDER)) goto retry; So in the unlikely scenario where the first call doesn't make the necessary headroom, and the shrinkers are the only thing that made forward progress, we would OOM prematurely. Not that an OOM would seem that far away in that scenario, anyway. But I remember long discussions with DavidR on probabilistic OOM regressions ;) > > I think the code should still be accounting for all pages that > > belong to the memcg being scanned that are reclaimed, not ignoring > > them altogether... > > 100% agree. Ideally I would want to: > - For pruned inodes: report all freed pages for global reclaim, and > only report pages charged to the memcg under reclaim for memcg > reclaim. This only happens on highmem systems at this point, as elsewhere populated inodes aren't on the shrinker LRUs anymore. We'd probably be ok with a comment noting the inaccuracy in the proactive reclaim stats for the time being, until somebody actually cares about that combination. > - For slab: report all freed pages for global reclaim, and only report > uncharged objcg pages from the memcg under reclaim for memcg reclaim. > > The only problem is that I thought people would think this is too much > complexity and not worth it. If people agree this should be the > approach to follow, I can prepare patches for this. I originally > implemented this for slab pages, but held off on sending it. I'd be curious to see the code!
