Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211001205657.815551-1-surenb@google.com> <20211001205657.815551-2-surenb@google.com>
 <20211001160830.700c36b32b736478000b3420@linux-foundation.org>
 <CAJuCfpGpMru4z=ZMezRQW56tHNjrWHU3jWhG3qzuXvuUytq-3w@mail.gmail.com>
 <CAJuCfpFWUXQ445VcqTcV1kNY3AWX=wB5iaeDAX_=+xZefjTUjg@mail.gmail.com> <20211006193940.c261f21fcd14b4b52aae1fbc@linux-foundation.org>
In-Reply-To: <20211006193940.c261f21fcd14b4b52aae1fbc@linux-foundation.org>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Wed, 6 Oct 2021 19:50:38 -0700
Message-ID: <CAJuCfpGMKPmVx8ADwFOyRPYfYKCuOHCUzr2o-vq0GZGSVj20Dw@mail.gmail.com>
Subject: Re: [PATCH v10 2/3] mm: add a field to store names for private
 anonymous memory
To:     Andrew Morton <akpm@linux-foundation.org>
Cc:     Colin Cross <ccross@google.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        Michal Hocko <mhocko@suse.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Kees Cook <keescook@chromium.org>,
        Matthew Wilcox <willy@infradead.org>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Randy Dunlap <rdunlap@infradead.org>,
        Kalesh Singh <kaleshsingh@google.com>,
        Peter Xu <peterx@redhat.com>, rppt@kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        vincenzo.frascino@arm.com,
        Chinwen Chang <chinwen.chang@mediatek.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Jann Horn <jannh@google.com>, apopple@nvidia.com,
        John Hubbard <jhubbard@nvidia.com>,
        Yu Zhao <yuzhao@google.com>, Will Deacon <will@kernel.org>,
        fenghua.yu@intel.com, thunder.leizhen@huawei.com,
        Hugh Dickins <hughd@google.com>, feng.tang@intel.com,
        Jason Gunthorpe <jgg@ziepe.ca>, Roman Gushchin <guro@fb.com>,
        Thomas Gleixner <tglx@linutronix.de>, krisman@collabora.com,
        Chris Hyser <chris.hyser@oracle.com>,
        Peter Collingbourne <pcc@google.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Jens Axboe <axboe@kernel.dk>, legion@kernel.org,
        Rolf Eike Beer <eb@emlix.com>,
        Cyrill Gorcunov <gorcunov@gmail.com>,
        Pavel Machek <pavel@ucw.cz>,
        Muchun Song <songmuchun@bytedance.com>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Thomas Cedeno <thomascedeno@google.com>, sashal@kernel.org,
        cxfcosmos@gmail.com, Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-mm <linux-mm@kvack.org>,
        kernel-team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Oct 6, 2021 at 7:39 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 4 Oct 2021 09:21:42 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > > > > The name pointers are not shared between vmas even if they contain the
> > > > > same name. The name pointer is stored in a union with fields that are
> > > > > only used on file-backed mappings, so it does not increase memory usage.
> > > > >
> > > > > The patch is based on the original patch developed by Colin Cross, more
> > > > > specifically on its latest version [1] posted upstream by Sumit Semwal.
> > > > > It used a userspace pointer to store vma names. In that design, name
> > > > > pointers could be shared between vmas. However during the last upstreaming
> > > > > attempt, Kees Cook raised concerns [2] about this approach and suggested
> > > > > to copy the name into kernel memory space, perform validity checks [3]
> > > > > and store as a string referenced from vm_area_struct.
> > > > > One big concern is about fork() performance which would need to strdup
> > > > > anonymous vma names. Dave Hansen suggested experimenting with worst-case
> > > > > scenario of forking a process with 64k vmas having longest possible names
> > > > > [4]. I ran this experiment on an ARM64 Android device and recorded a
> > > > > worst-case regression of almost 40% when forking such a process. This
> > > > > regression is addressed in the followup patch which replaces the pointer
> > > > > to a name with a refcounted structure that allows sharing the name pointer
> > > > > between vmas of the same name. Instead of duplicating the string during
> > > > > fork() or when splitting a vma it increments the refcount.
> > > >
> > > > Generally, the patch adds a bunch of code which a lot of users won't
> > > > want.  Did we bust a gut to reduce this impact?  Was a standalone
> > > > config setting considered?
> > >
> > > I didn't consider a standalone config for this feature because when
> > > not used it has no memory impact at runtime. As for the image size, I
> > > built Linus' ToT with and without this patchset with allmodconfig and
>
> allnoconfig would be more interesting.  People who want small kernels
> won't be using allmodconfig!

Sure, I will check that and report back.

>
> > > the sizes are:
> > > Without the patchset:
> > > $ size vmlinux
> > >    text    data     bss     dec     hex filename
> > > 40763556 58424519 29016228 128204303 7a43e0f vmlinux
> > >
> > > With the patchset:
> > > $ size vmlinux
> > >    text    data     bss     dec     hex filename
> > > 40765068 58424671 29016228 128205967 7a4448f vmlinux
> > >
> > > The increase seems quite small, so I'm not sure if it warrants a
> > > separate config option.
> >
> > Andrew, do you still think we need a separate CONFIG option? I fixed
> > the build issue when CONFIG_ADVISE_SYSCALLS=n and would like to post
> > the update but if you want to have a separate config then I can post
> > that together with the fix. Please let me know.
>
> I don't see much downside to the standalone option.  More complexity
> for developers/testers, I guess.  But such is life?

Sounds good to me. I will post a new version with a separate config if
we get over the objections of using numbers instead of strings.
Thanks!

>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>