Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220826150807.723137-1-glider@google.com> <20220826150807.723137-5-glider@google.com>
 <20220826211729.e65d52e7919fee5c34d22efc@linux-foundation.org>
 <CAG_fn=Xpva_yx8oG-xi7jqJyM2YLcjNda+8ZyQPGBMV411XgMQ@mail.gmail.com>
 <20220829122452.cce41f2754c4e063f3ae8b75@linux-foundation.org>
 <CAG_fn=X6eZ6Cdrv5pivcROHi3D8uymdgh+EbnFasBap2a=0LQQ@mail.gmail.com> <20220830150549.afa67340c2f5eb33ff9615f4@linux-foundation.org>
In-Reply-To: <20220830150549.afa67340c2f5eb33ff9615f4@linux-foundation.org>
From:   Yu Zhao <yuzhao@google.com>
Date:   Thu, 1 Sep 2022 17:44:03 -0600
Message-ID: <CAOUHufY91Eju-g1+xbUsGkGZ-cwBm78v+S_Air7Cp8mAnYJVYA@mail.gmail.com>
Subject: Re: [PATCH v5 04/44] x86: asm: instrument usercopy in get_user() and put_user()
To:     Andrew Morton <akpm@linux-foundation.org>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Alexander Potapenko <glider@google.com>,
        Marco Elver <elver@google.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Alexei Starovoitov <ast@kernel.org>,
        Andrey Konovalov <andreyknvl@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>, Borislav Petkov <bp@alien8.de>,
        Christoph Hellwig <hch@lst.de>,
        Christoph Lameter <cl@linux.com>,
        David Rientjes <rientjes@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Eric Dumazet <edumazet@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Ilya Leoshkevich <iii@linux.ibm.com>,
        Jens Axboe <axboe@kernel.dk>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Kees Cook <keescook@chromium.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Matthew Wilcox <willy@infradead.org>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        Pekka Enberg <penberg@kernel.org>,
        Petr Mladek <pmladek@suse.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Vegard Nossum <vegard.nossum@oracle.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        Linux-Arch <linux-arch@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 30, 2022 at 4:05 PM Andrew Morton <akpm@linux-foundation.org> wrote:
...
> Yu, that inclusion is regrettable.  I don't think mm_types.h is an
> appropriate site for implementing lru_gen_use_mm() anyway.  Adding a
> new header is always the right fix for these things.  I'd suggest
> adding a new mglru.h (or whatever) and putting most/all of the mglru
> material in there.
>
> Also, the addition to kernel/sched/core.c wasn't clearly changelogged,
> is uncommented and I doubt if the sched developers know about it, let
> alone reviewed it.  Please give them a heads-up.

Adding Ingo, Peter, Juri and Vincent.

I added lru_gen_use_mm() (one store operation) to context_switch() in
kernel/sched/core.c, and I would appreciate it if you could take a
look and let me know if you have any concerns:
https://lore.kernel.org/r/20220815071332.627393-9-yuzhao@google.com/

I'll resend the series in a week or so, and cc you when that happens.

> The addition looks fairly benign, but core context_switch() is the
> sort of thing which people get rather defensive about and putting
> mm-specific stuff in there might be challenged.  Some quantitative
> justification of this optimization would be appropriate.

The commit message (from the above link) touches on the theory only:

    This patch uses the following optimizations when walking page tables:
    1. It tracks the usage of mm_struct's between context switches so that
       page table walkers can skip processes that have been sleeping since
       the last iteration.

Let me expand on this.

TLDR: lru_gen_use_mm() introduces an extra store operation whenever
switching to a new mm_struct, which sets a flag for page reclaim to
clear.

For systems that are NOT under memory pressure:
1. This is a new overhead.
2. I don't think it's measurable, hence can't be the last straw.
3. Assume it can be measured, the belief is that underutilized systems
should be sacrificed (to some degree) for the greater good.

For systems that are under memory pressure:
1. When this flag is set on a mm_struct, page reclaim knows that this
mm_struct has been used since the last time it cleared this flag. So
it's worth checking out this mm_struct (to clear the accessed bit).
2. The similar idea has been used on Android and ChromeOS: when an app
or a tab goes to the background, these systems (conditionally) call
MADV_COLD. The majority of GUI applications don't implement this idea.
MGLRU opts to do it for the benefit of them. How it benefits server
applications is unknown (uninteresting).
3. This optimization benefits arm64 v8.2+ more than x86, since x86
supports the accessed bit in non-leaf entries and therefore the search
space can be reduced based on that. On a 4GB ARM system with 40 Chrome
tabs opened and 5 tabs in active use, this optimization improves page
table walk performance by about 5%. The overall benefit is small but
measurable under heavy memory pressure.
4. The idea can be reused by other MM components, e.g., khugepaged.

Thanks.