Re: Unnecessary mmap flags?
Date: Thu, 26 Jun 2014 17:01:23 -0700 From: Matthew Dempsky matt...@dempsky.org On Thu, Jun 26, 2014 at 12:28:18PM -0700, Matthew Dempsky wrote: I just reviewed our mmap(2) flags to compare them against Linux, FreeBSD, Solaris, and Darwin's flags. Of the flags listed below, none of them are specified by POSIX, and none of them do anything interesting on OpenBSD: MAP_COPY just gets rewritten to MAP_PRIVATE, and the rest are silently ignored by UVM. Feedback so far is the useless flags should go away. Diff below is a first step towards this: 1. MAP_COPY is redefined as an alias for MAP_PRIVATE, and the other useless MAP_* flags are redefined to 0. They're also hidden from the kernel to make sure no kernel code accidentally depends on them still. 2. Adds COMPAT_O55_MAP_COPY so we can stay binary compatible with any OpenBSD 5.5 programs that still use MAP_COPY (probably none, but it's trivial to do), and COMPAT_O55_MAP_NOOPMASK just to keep track of which bits were previously reserved for do-nothing flags. 3. Reshuffles the defines a little bit so the order makes more sense. Followup patch will add a deprecation warning for the MAP_* flags, but I think that'll need some ports testing, whereas this should be safe to commit now. ok? Losing the descriptions of the no-op flags is a bit unfortunate. Can you add those back? Index: sys/mman.h === RCS file: /home/matthew/cvs-mirror/cvs/src/sys/sys/mman.h,v retrieving revision 1.24 diff -u -p -r1.24 mman.h --- sys/mman.h13 Jun 2014 01:48:52 - 1.24 +++ sys/mman.h26 Jun 2014 23:54:28 - @@ -50,32 +50,43 @@ */ #define MAP_SHARED 0x0001 /* share changes */ #define MAP_PRIVATE 0x0002 /* changes are private */ -#define MAP_COPY0x0004 /* copy region at mmap time */ + +/* + * Mapping type + */ +#define MAP_FILE0x /* map from file (default) */ +#define MAP_ANON0x1000 /* allocated from memory, swap space */ /* * Other flags */ -#define MAP_FIXED0x0010 /* map addr must be exactly as requested */ -#define MAP_RENAME 0x0020 /* Sun: rename private pages to file */ -#define MAP_NORESERVE0x0040 /* Sun: don't reserve needed swap area */ -#define MAP_INHERIT 0x0080 /* region is retained after exec */ -#define MAP_NOEXTEND 0x0100 /* for MAP_FILE, don't change file size */ -#define MAP_HASSEMAPHORE 0x0200 /* region may contain semaphores */ -#define MAP_TRYFIXED 0x0400 /* attempt hint address, even within heap */ +#define MAP_FIXED 0x0010 /* map addr must be exactly as requested */ +#define __MAP_NOREPLACE 0x0800 /* fail if address not available */ -#define __MAP_NOREPLACE 0x0800 /* fail if address not available */ +#ifdef _KERNEL +#define COMPAT_O55_MAP_COPY 0x0004 /* alias for MAP_PRIVATE */ +#define COMPAT_O55_MAP_NOOPMASK 0x07e0 /* formerly reserved flag bits */ +#endif + +#define MAP_FLAGMASK0x1ff7 +#ifndef _KERNEL /* - * Error return from mmap() + * Deprecated flags with no significant meaning on OpenBSD. */ -#define MAP_FAILED ((void *)-1) +#define MAP_COPYMAP_PRIVATE +#define MAP_HASSEMAPHORE0 +#define MAP_INHERIT 0 +#define MAP_NOEXTEND0 +#define MAP_NORESERVE 0 +#define MAP_RENAME 0 +#define MAP_TRYFIXED0 +#endif /* - * Mapping type + * Error return from mmap() */ -#define MAP_FILE0x /* map from file (default) */ -#define MAP_ANON0x1000 /* allocated from memory, swap space */ -#define MAP_FLAGMASK0x1ff7 +#define MAP_FAILED ((void *)-1) /* * POSIX memory advisory values. Index: uvm/uvm_mmap.c === RCS file: /home/matthew/cvs-mirror/cvs/src/sys/uvm/uvm_mmap.c,v retrieving revision 1.94 diff -u -p -r1.94 uvm_mmap.c --- uvm/uvm_mmap.c13 Apr 2014 23:14:15 - 1.94 +++ uvm/uvm_mmap.c26 Jun 2014 23:49:39 - @@ -345,8 +345,8 @@ sys_mmap(struct proc *p, void *v, regist return (EINVAL); if ((flags MAP_FLAGMASK) != flags) return (EINVAL); - if (flags MAP_COPY) - flags = (flags ~MAP_COPY) | MAP_PRIVATE; + if (flags COMPAT_O55_MAP_COPY) + flags = (flags ~COMPAT_O55_MAP_COPY) | MAP_PRIVATE; if ((flags (MAP_SHARED|MAP_PRIVATE)) == (MAP_SHARED|MAP_PRIVATE)) return (EINVAL); if ((flags (MAP_FIXED|__MAP_NOREPLACE)) == __MAP_NOREPLACE)
Re: Unnecessary mmap flags?
Date: Thu, 26 Jun 2014 12:28:18 -0700 From: Matthew Dempsky matt...@dempsky.org I just reviewed our mmap(2) flags to compare them against Linux, FreeBSD, Solaris, and Darwin's flags. Of the flags listed below, none of them are specified by POSIX, and none of them do anything interesting on OpenBSD: MAP_COPY just gets rewritten to MAP_PRIVATE, and the rest are silently ignored by UVM. Linux FreeBSD Solaris Darwin MAP_COPY no YES*no YES* MAP_RENAMEno YES*YES*YES* MAP_NORESERVE YES YES*YES YES* MAP_INHERIT no YES** no no MAP_NOEXTEND no no no YES* MAP_HASSEMAPHORE no YES*** no YES*** MAP_TRYFIXED no no no no MAP_TRYFIXED is a NetBSD'ism. The others are inherited straight from 4.4BSD. So MAP_NORESERVE is perhaps necessary/worth keeping around, but the others seem like candidates for removal if nothing in ports needs them. I think that MAP_NORESERVE should indeed be kept. MAP_HASSEMAPHORE is used in rthread_sem.c, but it doesn't do anything, so I suspect it's just cargo culting based on man page misinformation? Are there architectures that actually have restrictions on semaphore memory? There architectures where atomic instructions only work on pages with certain caching attributes. Those attributes tend to be the default attributes though, so there is not much value in retaining this flag.
Re: Unnecessary mmap flags?
On Fri, Jun 27, 2014 at 02:46:01PM +0200, Mark Kettenis wrote: Losing the descriptions of the no-op flags is a bit unfortunate. Can you add those back? Okay, restored them below. Also tested that kdump can handle this change gracefully. In this diff I've also moved MAP_FILE down to the legacy flags section. I don't expect we'll want to remove it, but strictly speaking it *is* just a legacy flag: POSIX doesn't define MAP_FILE and instead just mandates the map from file semantics even without any flag. ok? Index: sys/sys/mman.h === RCS file: /home/matthew/cvs-mirror/cvs/src/sys/sys/mman.h,v retrieving revision 1.24 diff -u -p -r1.24 mman.h --- sys/sys/mman.h 13 Jun 2014 01:48:52 - 1.24 +++ sys/sys/mman.h 27 Jun 2014 17:26:32 - @@ -50,32 +50,49 @@ */ #defineMAP_SHARED 0x0001 /* share changes */ #defineMAP_PRIVATE 0x0002 /* changes are private */ -#defineMAP_COPY0x0004 /* copy region at mmap time */ /* * Other flags */ -#defineMAP_FIXED0x0010 /* map addr must be exactly as requested */ -#defineMAP_RENAME 0x0020 /* Sun: rename private pages to file */ -#defineMAP_NORESERVE0x0040 /* Sun: don't reserve needed swap area */ -#defineMAP_INHERIT 0x0080 /* region is retained after exec */ -#defineMAP_NOEXTEND 0x0100 /* for MAP_FILE, don't change file size */ -#defineMAP_HASSEMAPHORE 0x0200 /* region may contain semaphores */ -#defineMAP_TRYFIXED 0x0400 /* attempt hint address, even within heap */ +#defineMAP_FIXED 0x0010 /* map addr must be exactly as requested */ +#define__MAP_NOREPLACE 0x0800 /* fail if address not available */ +#defineMAP_ANON0x1000 /* allocated from memory, swap space */ -#define__MAP_NOREPLACE 0x0800 /* fail if address not available */ +#defineMAP_FLAGMASK0x1ff7 +#ifdef _KERNEL /* - * Error return from mmap() + * Backwards compat for OpenBSD 5.5. + * TODO: Remove after OpenBSD 5.7 release. */ -#define MAP_FAILED ((void *)-1) +#defineMAP_OLDCOPY 0x0004 /* alias for MAP_PRIVATE */ +#defineMAP_OLDRENAME 0x0020 +#defineMAP_OLDNORESERVE0x0040 +#defineMAP_OLDINHERIT 0x0080 +#defineMAP_OLDNOEXTEND 0x0100 +#defineMAP_OLDHASSEMAPHORE 0x0200 +#defineMAP_OLDTRYFIXED 0x0400 +#endif +#ifndef _KERNEL /* - * Mapping type + * Legacy defines for userland source compatibility. + * Can be removed once no longer needed in base and ports. */ -#defineMAP_FILE0x /* map from file (default) */ -#defineMAP_ANON0x1000 /* allocated from memory, swap space */ -#defineMAP_FLAGMASK0x1ff7 +#defineMAP_COPYMAP_PRIVATE /* copy region at mmap time */ +#defineMAP_FILE0 /* map from file (default) */ +#defineMAP_HASSEMAPHORE0 /* region may contain semaphores */ +#defineMAP_INHERIT 0 /* region is retained after exec */ +#defineMAP_NOEXTEND0 /* for MAP_FILE, don't change file size */ +#defineMAP_NORESERVE 0 /* Sun: don't reserve needed swap area */ +#defineMAP_RENAME 0 /* Sun: rename private pages to file */ +#defineMAP_TRYFIXED0 /* attempt hint address, even within heap */ +#endif + +/* + * Error return from mmap() + */ +#define MAP_FAILED ((void *)-1) /* * POSIX memory advisory values. Index: usr.bin/kdump/mksubr === RCS file: /home/matthew/cvs-mirror/cvs/src/usr.bin/kdump/mksubr,v retrieving revision 1.18 diff -u -p -r1.18 mksubr --- usr.bin/kdump/mksubr21 Dec 2013 07:32:35 - 1.18 +++ usr.bin/kdump/mksubr27 Jun 2014 17:08:46 - @@ -241,7 +241,9 @@ cat _EOF_ #include sys/fcntl.h #include sys/stat.h #include sys/unistd.h +#define _KERNEL #include sys/mman.h +#undef _KERNEL #include sys/wait.h #include sys/proc.h #define _KERNEL
Re: Unnecessary mmap flags?
MAP_HASSEMAPHORE is used in rthread_sem.c, but it doesn't do anything, so I suspect it's just cargo culting based on man page misinformation? Are there architectures that actually have restrictions on semaphore memory? There architectures where atomic instructions only work on pages with certain caching attributes. Those attributes tend to be the default attributes though, so there is not much value in retaining this flag. Whether such architectures exist and want to do atomic handling is one thing. The real question is if any applications set the flag. Looks like no.
Re: Unnecessary mmap flags?
On 06/26/2014 03:28 PM, Matthew Dempsky wrote: I just reviewed our mmap(2) flags to compare them against Linux, FreeBSD, Solaris, and Darwin's flags. Of the flags listed below, none of them are specified by POSIX, and none of them do anything interesting on OpenBSD: MAP_COPY just gets rewritten to MAP_PRIVATE, and the rest are silently ignored by UVM. Linux FreeBSD Solaris Darwin MAP_COPYno YES*no YES* MAP_RENAME no YES*YES*YES* MAP_NORESERVE YES YES*YES YES* MAP_INHERIT no YES** no no MAP_NOEXTENDno no no YES* MAP_HASSEMAPHOREno YES*** no YES*** MAP_TRYFIXEDno no no no * These are defined in the OS's sys/mman.h, but are undocumented in their mmap(2) manual, and behave the same as on OpenBSD (i.e., MAP_COPY is an alias for MAP_PRIVATE; the others have no effect). ** MAP_INHERIT is documented in FreeBSD's mmap(2) manual (as This flag never operated as advertised, which is true on OpenBSD too), but not defined in their sys/mman.h. *** MAP_HASSEMAPHORE is documented in FreeBSD and Darwin's mmap(2) manuals and defined in their sys/mman.h, but has no effects on either's VM behavior. So MAP_NORESERVE is perhaps necessary/worth keeping around, but the others seem like candidates for removal if nothing in ports needs them. MAP_HASSEMAPHORE is used in rthread_sem.c, but it doesn't do anything, so I suspect it's just cargo culting based on man page misinformation? Are there architectures that actually have restrictions on semaphore memory? Sun allowed requests for shared memory to be uncached. This was used for DMA and (IIRC) interprocessor communication since adding sufficient memory barriers was painful. I don't remember how to make that request. Geoff Steckel
Re: Unnecessary mmap flags?
On Thu, Jun 26, 2014 at 12:28:18PM -0700, Matthew Dempsky wrote: I just reviewed our mmap(2) flags to compare them against Linux, FreeBSD, Solaris, and Darwin's flags. Of the flags listed below, none of them are specified by POSIX, and none of them do anything interesting on OpenBSD: MAP_COPY just gets rewritten to MAP_PRIVATE, and the rest are silently ignored by UVM. Feedback so far is the useless flags should go away. Diff below is a first step towards this: 1. MAP_COPY is redefined as an alias for MAP_PRIVATE, and the other useless MAP_* flags are redefined to 0. They're also hidden from the kernel to make sure no kernel code accidentally depends on them still. 2. Adds COMPAT_O55_MAP_COPY so we can stay binary compatible with any OpenBSD 5.5 programs that still use MAP_COPY (probably none, but it's trivial to do), and COMPAT_O55_MAP_NOOPMASK just to keep track of which bits were previously reserved for do-nothing flags. 3. Reshuffles the defines a little bit so the order makes more sense. Followup patch will add a deprecation warning for the MAP_* flags, but I think that'll need some ports testing, whereas this should be safe to commit now. ok? Index: sys/mman.h === RCS file: /home/matthew/cvs-mirror/cvs/src/sys/sys/mman.h,v retrieving revision 1.24 diff -u -p -r1.24 mman.h --- sys/mman.h 13 Jun 2014 01:48:52 - 1.24 +++ sys/mman.h 26 Jun 2014 23:54:28 - @@ -50,32 +50,43 @@ */ #defineMAP_SHARED 0x0001 /* share changes */ #defineMAP_PRIVATE 0x0002 /* changes are private */ -#defineMAP_COPY0x0004 /* copy region at mmap time */ + +/* + * Mapping type + */ +#defineMAP_FILE0x /* map from file (default) */ +#defineMAP_ANON0x1000 /* allocated from memory, swap space */ /* * Other flags */ -#defineMAP_FIXED0x0010 /* map addr must be exactly as requested */ -#defineMAP_RENAME 0x0020 /* Sun: rename private pages to file */ -#defineMAP_NORESERVE0x0040 /* Sun: don't reserve needed swap area */ -#defineMAP_INHERIT 0x0080 /* region is retained after exec */ -#defineMAP_NOEXTEND 0x0100 /* for MAP_FILE, don't change file size */ -#defineMAP_HASSEMAPHORE 0x0200 /* region may contain semaphores */ -#defineMAP_TRYFIXED 0x0400 /* attempt hint address, even within heap */ +#defineMAP_FIXED 0x0010 /* map addr must be exactly as requested */ +#define__MAP_NOREPLACE 0x0800 /* fail if address not available */ -#define__MAP_NOREPLACE 0x0800 /* fail if address not available */ +#ifdef _KERNEL +#define COMPAT_O55_MAP_COPY0x0004 /* alias for MAP_PRIVATE */ +#define COMPAT_O55_MAP_NOOPMASK0x07e0 /* formerly reserved flag bits */ +#endif + +#defineMAP_FLAGMASK0x1ff7 +#ifndef _KERNEL /* - * Error return from mmap() + * Deprecated flags with no significant meaning on OpenBSD. */ -#define MAP_FAILED ((void *)-1) +#defineMAP_COPYMAP_PRIVATE +#defineMAP_HASSEMAPHORE0 +#defineMAP_INHERIT 0 +#defineMAP_NOEXTEND0 +#defineMAP_NORESERVE 0 +#defineMAP_RENAME 0 +#defineMAP_TRYFIXED0 +#endif /* - * Mapping type + * Error return from mmap() */ -#defineMAP_FILE0x /* map from file (default) */ -#defineMAP_ANON0x1000 /* allocated from memory, swap space */ -#defineMAP_FLAGMASK0x1ff7 +#define MAP_FAILED ((void *)-1) /* * POSIX memory advisory values. Index: uvm/uvm_mmap.c === RCS file: /home/matthew/cvs-mirror/cvs/src/sys/uvm/uvm_mmap.c,v retrieving revision 1.94 diff -u -p -r1.94 uvm_mmap.c --- uvm/uvm_mmap.c 13 Apr 2014 23:14:15 - 1.94 +++ uvm/uvm_mmap.c 26 Jun 2014 23:49:39 - @@ -345,8 +345,8 @@ sys_mmap(struct proc *p, void *v, regist return (EINVAL); if ((flags MAP_FLAGMASK) != flags) return (EINVAL); - if (flags MAP_COPY) - flags = (flags ~MAP_COPY) | MAP_PRIVATE; + if (flags COMPAT_O55_MAP_COPY) + flags = (flags ~COMPAT_O55_MAP_COPY) | MAP_PRIVATE; if ((flags (MAP_SHARED|MAP_PRIVATE)) == (MAP_SHARED|MAP_PRIVATE)) return (EINVAL); if ((flags (MAP_FIXED|__MAP_NOREPLACE)) == __MAP_NOREPLACE)
Re: Unnecessary mmap flags?
1. MAP_COPY is redefined as an alias for MAP_PRIVATE, and the other useless MAP_* flags are redefined to 0. They're also hidden from the kernel to make sure no kernel code accidentally depends on them still. 2. Adds COMPAT_O55_MAP_COPY so we can stay binary compatible with any OpenBSD 5.5 programs that still use MAP_COPY (probably none, but it's trivial to do), and COMPAT_O55_MAP_NOOPMASK just to keep track of which bits were previously reserved for do-nothing flags. Yes, then we can remove the kernel support soon. We need to remember to do that.. sometimes such things live far longer than the ABI requires. I suspect the new name might hurt kdump support (grep for mmapflagsname).
