On Sat, Apr 10, 2021 at 12:00 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > On Fri, Apr 09, 2021 at 08:29:45PM +0800, Muchun Song wrote: > > We already have a helper lruvec_memcg() to get the memcg from lruvec, we > > do not need to do it ourselves in the lruvec_holds_page_lru_lock(). So use > > lruvec_memcg() instead. And if mem_cgroup_disabled() returns false, the > > page_memcg(page) (the LRU pages) cannot be NULL. So remove the odd logic > > of "memcg = page_memcg(page) ? : root_mem_cgroup". And use lruvec_pgdat > > to simplify the code. We can have a single definition for this function > > that works for !CONFIG_MEMCG, CONFIG_MEMCG + mem_cgroup_disabled() and > > CONFIG_MEMCG. > > > > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx> > > Looks good to me. > > Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx> Thanks for your review. > > If you haven't done so yet, please make sure to explicitly test with > all three config combinations, just because the dummy abstractions for > memcg disabled or compiled out tend to be paper thin and don't always > behave the way you might expect when you do more complicated things. I have tested. There is no problem. Thanks :-) > > Something like > > boot > echo sparsefile >/dev/null (> ram size to fill memory and reclaim) > echo 1 >/proc/sys/vm/compact_memory > > should exercise this new function in a couple of important scenarios.
