On Wed, Oct 6, 2021 at 7:39 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote: > > On Mon, 4 Oct 2021 09:21:42 -0700 Suren Baghdasaryan <surenb@xxxxxxxxxx> 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@xxxxxxxxxxx. >
