| Message ID | 1437773325-8623-6-git-send-email-emunson@akamai.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Jul 24, 2015 at 05:28:43PM -0400, Eric B Munson wrote: > The cost of faulting in all memory to be locked can be very high when > working with large mappings. If only portions of the mapping will be > used this can incur a high penalty for locking. > > Now that we have the new VMA flag for the locked but not present state, > expose it as an mmap option like MAP_LOCKED -> VM_LOCKED. As I mentioned before, I don't think this interface is justified. MAP_LOCKED has known issues[1]. The MAP_LOCKED problem is not necessary affects MAP_LOCKONFAULT, but still. Let's not add new interface unless it's demonstrably useful. [1] http://lkml.kernel.org/g/20150114095019.GC4706@dhcp22.suse.cz
On Mon, 27 Jul 2015, Kirill A. Shutemov wrote: > On Fri, Jul 24, 2015 at 05:28:43PM -0400, Eric B Munson wrote: > > The cost of faulting in all memory to be locked can be very high when > > working with large mappings. If only portions of the mapping will be > > used this can incur a high penalty for locking. > > > > Now that we have the new VMA flag for the locked but not present state, > > expose it as an mmap option like MAP_LOCKED -> VM_LOCKED. > > As I mentioned before, I don't think this interface is justified. > > MAP_LOCKED has known issues[1]. The MAP_LOCKED problem is not necessary > affects MAP_LOCKONFAULT, but still. > > Let's not add new interface unless it's demonstrably useful. > > [1] http://lkml.kernel.org/g/20150114095019.GC4706@dhcp22.suse.cz I understand and should have been more explicit. This patch is still included becuase I have an internal user that wants to see it added. The problem discussed in the thread you point out does not affect MAP_LOCKONFAULT because we do not attempt to populate the region with MAP_LOCKONFAULT. As I told Vlastimil, if this is a hard NAK with the patch I can work with that. Otherwise I prefer it stays.
On Mon, Jul 27, 2015 at 09:41:26AM -0400, Eric B Munson wrote: > On Mon, 27 Jul 2015, Kirill A. Shutemov wrote: > > > On Fri, Jul 24, 2015 at 05:28:43PM -0400, Eric B Munson wrote: > > > The cost of faulting in all memory to be locked can be very high when > > > working with large mappings. If only portions of the mapping will be > > > used this can incur a high penalty for locking. > > > > > > Now that we have the new VMA flag for the locked but not present state, > > > expose it as an mmap option like MAP_LOCKED -> VM_LOCKED. > > > > As I mentioned before, I don't think this interface is justified. > > > > MAP_LOCKED has known issues[1]. The MAP_LOCKED problem is not necessary > > affects MAP_LOCKONFAULT, but still. > > > > Let's not add new interface unless it's demonstrably useful. > > > > [1] http://lkml.kernel.org/g/20150114095019.GC4706@dhcp22.suse.cz > > I understand and should have been more explicit. This patch is still > included becuase I have an internal user that wants to see it added. > The problem discussed in the thread you point out does not affect > MAP_LOCKONFAULT because we do not attempt to populate the region with > MAP_LOCKONFAULT. > > As I told Vlastimil, if this is a hard NAK with the patch I can work > with that. Otherwise I prefer it stays. That's not how it works. Once an ABI added to the kernel it stays there practically forever. Therefore it must be useful to justify maintenance cost. I don't see it demonstrated. So, NAK.
On Mon, 27 Jul 2015, Kirill A. Shutemov wrote: > On Mon, Jul 27, 2015 at 09:41:26AM -0400, Eric B Munson wrote: > > On Mon, 27 Jul 2015, Kirill A. Shutemov wrote: > > > > > On Fri, Jul 24, 2015 at 05:28:43PM -0400, Eric B Munson wrote: > > > > The cost of faulting in all memory to be locked can be very high when > > > > working with large mappings. If only portions of the mapping will be > > > > used this can incur a high penalty for locking. > > > > > > > > Now that we have the new VMA flag for the locked but not present state, > > > > expose it as an mmap option like MAP_LOCKED -> VM_LOCKED. > > > > > > As I mentioned before, I don't think this interface is justified. > > > > > > MAP_LOCKED has known issues[1]. The MAP_LOCKED problem is not necessary > > > affects MAP_LOCKONFAULT, but still. > > > > > > Let's not add new interface unless it's demonstrably useful. > > > > > > [1] http://lkml.kernel.org/g/20150114095019.GC4706@dhcp22.suse.cz > > > > I understand and should have been more explicit. This patch is still > > included becuase I have an internal user that wants to see it added. > > The problem discussed in the thread you point out does not affect > > MAP_LOCKONFAULT because we do not attempt to populate the region with > > MAP_LOCKONFAULT. > > > > As I told Vlastimil, if this is a hard NAK with the patch I can work > > with that. Otherwise I prefer it stays. > > That's not how it works. I am not sure what you mean here. I have a user that will find this useful and MAP_LOCKONFAULT does not suffer from the problem you point out. I do not understand your NAK but thank you for explicit about it. > > Once an ABI added to the kernel it stays there practically forever. > Therefore it must be useful to justify maintenance cost. I don't see it > demonstrated. I understand this, and I get that you do not like MAP_LOCKED, but I do not see how your dislike for MAP_LOCKED means that this would not be useful. > > So, NAK. > V6 will not have the new mmap flag unless there is someone else that speaks up in favor of keeping it.
diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h index 77ae8db..3f80ca4 100644 --- a/arch/alpha/include/uapi/asm/mman.h +++ b/arch/alpha/include/uapi/asm/mman.h @@ -30,6 +30,7 @@ #define MAP_NONBLOCK 0x40000 /* do not block on IO */ #define MAP_STACK 0x80000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x100000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x200000 /* Lock pages after they are faulted in, do not prefault */ #define MS_ASYNC 1 /* sync memory asynchronously */ #define MS_SYNC 2 /* synchronous memory sync */ diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h index 71ed81d..905c1ea 100644 --- a/arch/mips/include/uapi/asm/mman.h +++ b/arch/mips/include/uapi/asm/mman.h @@ -48,6 +48,7 @@ #define MAP_NONBLOCK 0x20000 /* do not block on IO */ #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x100000 /* Lock pages after they are faulted in, do not prefault */ /* * Flags for msync diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h index c0871ce..c4695f6 100644 --- a/arch/parisc/include/uapi/asm/mman.h +++ b/arch/parisc/include/uapi/asm/mman.h @@ -24,6 +24,7 @@ #define MAP_NONBLOCK 0x20000 /* do not block on IO */ #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x100000 /* Lock pages after they are faulted in, do not prefault */ #define MS_SYNC 1 /* synchronous memory sync */ #define MS_ASYNC 2 /* sync memory asynchronously */ diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h index f93f7eb..40a3fda 100644 --- a/arch/powerpc/include/uapi/asm/mman.h +++ b/arch/powerpc/include/uapi/asm/mman.h @@ -31,5 +31,6 @@ #define MAP_NONBLOCK 0x10000 /* do not block on IO */ #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x80000 /* Lock pages after they are faulted in, do not prefault */ #endif /* _UAPI_ASM_POWERPC_MMAN_H */ diff --git a/arch/sparc/include/uapi/asm/mman.h b/arch/sparc/include/uapi/asm/mman.h index 8cd2ebc..f66efa6 100644 --- a/arch/sparc/include/uapi/asm/mman.h +++ b/arch/sparc/include/uapi/asm/mman.h @@ -26,6 +26,7 @@ #define MAP_NONBLOCK 0x10000 /* do not block on IO */ #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x80000 /* Lock pages after they are faulted in, do not prefault */ #endif /* _UAPI__SPARC_MMAN_H__ */ diff --git a/arch/tile/include/uapi/asm/mman.h b/arch/tile/include/uapi/asm/mman.h index acdd013..800e5c3 100644 --- a/arch/tile/include/uapi/asm/mman.h +++ b/arch/tile/include/uapi/asm/mman.h @@ -29,6 +29,7 @@ #define MAP_DENYWRITE 0x0800 /* ETXTBSY */ #define MAP_EXECUTABLE 0x1000 /* mark it as an executable */ #define MAP_HUGETLB 0x4000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x100000 /* Lock pages after they are faulted in, do not prefault */ /* diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h index 5725a15..689e1f2 100644 --- a/arch/xtensa/include/uapi/asm/mman.h +++ b/arch/xtensa/include/uapi/asm/mman.h @@ -55,6 +55,7 @@ #define MAP_NONBLOCK 0x20000 /* do not block on IO */ #define MAP_STACK 0x40000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x80000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x100000 /* Lock pages after they are faulted in, do not prefault */ #ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED # define MAP_UNINITIALIZED 0x4000000 /* For anonymous mmap, memory could be * uninitialized */ diff --git a/include/linux/mman.h b/include/linux/mman.h index 16373c8..8243268 100644 --- a/include/linux/mman.h +++ b/include/linux/mman.h @@ -86,7 +86,8 @@ calc_vm_flag_bits(unsigned long flags) { return _calc_vm_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN ) | _calc_vm_trans(flags, MAP_DENYWRITE, VM_DENYWRITE ) | - _calc_vm_trans(flags, MAP_LOCKED, VM_LOCKED ); + _calc_vm_trans(flags, MAP_LOCKED, VM_LOCKED ) | + _calc_vm_trans(flags, MAP_LOCKONFAULT,VM_LOCKONFAULT | VM_LOCKED); } unsigned long vm_commit_limit(void); diff --git a/include/uapi/asm-generic/mman.h b/include/uapi/asm-generic/mman.h index 555aab0..007b784 100644 --- a/include/uapi/asm-generic/mman.h +++ b/include/uapi/asm-generic/mman.h @@ -12,6 +12,7 @@ #define MAP_NONBLOCK 0x10000 /* do not block on IO */ #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */ #define MAP_HUGETLB 0x40000 /* create a huge page mapping */ +#define MAP_LOCKONFAULT 0x80000 /* Lock pages after they are faulted in, do not prefault */ /* Bits [26:31] are reserved, see mman-common.h for MAP_HUGETLB usage */ diff --git a/kernel/events/core.c b/kernel/events/core.c index d3dae34..ec039f7 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5815,7 +5815,8 @@ static void perf_event_mmap_event(struct perf_mmap_event *mmap_event) if (vma->vm_flags & VM_MAYEXEC) flags |= MAP_EXECUTABLE; if (vma->vm_flags & VM_LOCKED) - flags |= MAP_LOCKED; + flags |= (vma->vm_flags & VM_LOCKONFAULT ? + MAP_LOCKONFAULT : MAP_LOCKED); if (vma->vm_flags & VM_HUGETLB) flags |= MAP_HUGETLB; diff --git a/mm/mmap.c b/mm/mmap.c index bdbefc3..56a842d 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1261,6 +1261,10 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr, if (!len) return -EINVAL; + if ((flags & (MAP_LOCKED | MAP_LOCKONFAULT)) == + (MAP_LOCKED | MAP_LOCKONFAULT)) + return -EINVAL; + /* * Does the application expect PROT_READ to imply PROT_EXEC? * @@ -1301,7 +1305,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr, vm_flags = calc_vm_prot_bits(prot) | calc_vm_flag_bits(flags) | mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC; - if (flags & MAP_LOCKED) + if (flags & (MAP_LOCKED | MAP_LOCKONFAULT)) if (!can_do_mlock()) return -EPERM; @@ -2674,7 +2678,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, flags &= MAP_NONBLOCK; flags |= MAP_SHARED | MAP_FIXED | MAP_POPULATE; if (vma->vm_flags & VM_LOCKED) { - flags |= MAP_LOCKED; + flags |= (vma->vm_flags & VM_LOCKONFAULT ? MAP_LOCKONFAULT : MAP_LOCKED); /* drop PG_Mlocked flag for over-mapped range */ munlock_vma_pages_range(vma, start, start + size); }
The cost of faulting in all memory to be locked can be very high when working with large mappings. If only portions of the mapping will be used this can incur a high penalty for locking. Now that we have the new VMA flag for the locked but not present state, expose it as an mmap option like MAP_LOCKED -> VM_LOCKED. Signed-off-by: Eric B Munson <emunson@akamai.com> Cc: Michal Hocko <mhocko@suse.cz> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Paul Gortmaker <paul.gortmaker@windriver.com> Cc: Chris Metcalf <cmetcalf@ezchip.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: linux-alpha@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-mips@linux-mips.org Cc: linux-parisc@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Cc: sparclinux@vger.kernel.org Cc: linux-xtensa@linux-xtensa.org Cc: linux-mm@kvack.org Cc: linux-arch@vger.kernel.org Cc: linux-api@vger.kernel.org --- arch/alpha/include/uapi/asm/mman.h | 1 + arch/mips/include/uapi/asm/mman.h | 1 + arch/parisc/include/uapi/asm/mman.h | 1 + arch/powerpc/include/uapi/asm/mman.h | 1 + arch/sparc/include/uapi/asm/mman.h | 1 + arch/tile/include/uapi/asm/mman.h | 1 + arch/xtensa/include/uapi/asm/mman.h | 1 + include/linux/mman.h | 3 ++- include/uapi/asm-generic/mman.h | 1 + kernel/events/core.c | 3 ++- mm/mmap.c | 8 ++++++-- 11 files changed, 18 insertions(+), 4 deletions(-)