linux-kernel - Re: [PATCH v4 0/3] Ignore non-LRU-based reclaim in memcg reclaim

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2023 14:49:13 -0700
From:   Yosry Ahmed <yosryahmed@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        "Darrick J. Wong" <djwong@...nel.org>,
        Christoph Lameter <cl@...ux.com>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Roman Gushchin <roman.gushchin@...ux.dev>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>,
        "Matthew Wilcox (Oracle)" <willy@...radead.org>,
        Miaohe Lin <linmiaohe@...wei.com>,
        David Hildenbrand <david@...hat.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Peter Xu <peterx@...hat.com>, NeilBrown <neilb@...e.de>,
        Shakeel Butt <shakeelb@...gle.com>,
        Michal Hocko <mhocko@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
        Dave Chinner <david@...morbit.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-xfs@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v4 0/3] Ignore non-LRU-based reclaim in memcg reclaim

On Tue, Apr 4, 2023 at 2:38 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
>
> On Tue,  4 Apr 2023 00:13:50 +0000 Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> > Upon running some proactive reclaim tests using memory.reclaim, we
> > noticed some tests flaking where writing to memory.reclaim would be
> > successful even though we did not reclaim the requested amount fully.
> > Looking further into it, I discovered that *sometimes* we over-report
> > the number of reclaimed pages in memcg reclaim.
> >
> > Reclaimed pages through other means than LRU-based reclaim are tracked
> > through reclaim_state in struct scan_control, which is stashed in
> > current task_struct. These pages are added to the number of reclaimed
> > pages through LRUs. For memcg reclaim, these pages generally cannot be
> > linked to the memcg under reclaim and can cause an overestimated count
> > of reclaimed pages. This short series tries to address that.
> >
> > Patches 1-2 are just refactoring, they add helpers that wrap some
> > operations on current->reclaim_state, and rename
> > reclaim_state->reclaimed_slab to reclaim_state->reclaimed.
> >
> > Patch 3 ignores pages reclaimed outside of LRU reclaim in memcg reclaim.
> > The pages are uncharged anyway, so even if we end up under-reporting
> > reclaimed pages we will still succeed in making progress during
> > charging.
> >
> > Do not let the diff stat deceive you, the core of this series is patch 3,
> > which has one line of code change. All the rest is refactoring and one
> > huge comment.
> >
>
> Wouldn't it be better to do this as a single one-line patch for
> backportability?  Then all the refactoring etcetera can be added on
> later.

Without refactoring the code that adds reclaim_state->reclaimed to
scan_control->nr_reclaimed into a helper (flush_reclaim_state()), the
change would need to be done in two places instead of one, and I
wouldn't know where to put the huge comment.

One thing that I can do is break down patch 2 into two patches, one
that adds the flush_reclaim_state() helper, and one that adds the
mm_account_reclaimed_pages() helper.

The series would be:
Patch 1: move set_task_reclaim_state() near other helpers
Patch 2: introduce mm_account_reclaimed_pages()
Patch 3: introduce flush_reclaim_state()
Patch 4: add the one-line change (and the huge comment) to flush_reclaim_state()

Backports need only to take patches 3 & 4 (which would be localized to
mm/vmscan.c), as patches 1 & 2 would be purely cosmetic with no
dependency from patches 3 & 4. For the current series, patch 1 is not
needed anyway. So this change would basically save backporters the
part of patch 2 that is outside of mm/vmscan.c.

If you think this would be useful I can send a v5 with patch 2 broken
down into two patches.