On Wed, Oct 6, 2021 at 7:39 PM Andrew Morton [off-list ref] wrote:

On Mon, 4 Oct 2021 09:21:42 -0700 Suren Baghdasaryan [off-list ref] wrote:

quoted

quoted

quoted

quoted

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.

quoted

quoted

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.