Re: [PATCH] remove duplicate asm/mman.h files

Re: [PATCH] remove duplicate asm/mman.h files

[Arnd Bergmann]

On Friday 18 September 2009, David Rientjes wrote:
> On Fri, 18 Sep 2009, Arnd Bergmann wrote:
>
> > -#define MCL_CURRENT	1		/* lock all current mappings */
> > -#define MCL_FUTURE	2		/* lock all future mappings */
> > +#define MAP_GROWSUP	0x0200		/* register stack-like segment */
> >  
> >  #ifdef __KERNEL__
> >  #ifndef __ASSEMBLY__
> 
> ia64 doesn't use MAP_GROWSUP, so it's probably not necessary to carry it 
> along with your cleanup.

ia64 is the only architecture defining it, nobody uses it in the kernel.
If the ia64 maintainers want to remove it in a separate patch, that
would probably be a good idea.

I tried not to change the ABI in any way in my patch, and there is
a theoretical possibility that some user space program on ia64 currently
depends on that definition.

	Arnd <><

Re: [PATCH] remove duplicate asm/mman.h files

[David Rientjes]

On Mon, 21 Sep 2009, Arnd Bergmann wrote:

> > > -#define MCL_CURRENT	1		/* lock all current mappings */
> > > -#define MCL_FUTURE	2		/* lock all future mappings */
> > > +#define MAP_GROWSUP	0x0200		/* register stack-like segment */
> > >  
> > >  #ifdef __KERNEL__
> > >  #ifndef __ASSEMBLY__
> > 
> > ia64 doesn't use MAP_GROWSUP, so it's probably not necessary to carry it 
> > along with your cleanup.
> 
> ia64 is the only architecture defining it, nobody uses it in the kernel.
> If the ia64 maintainers want to remove it in a separate patch, that
> would probably be a good idea.
> 

I'll do it then.

> I tried not to change the ABI in any way in my patch, and there is
> a theoretical possibility that some user space program on ia64 currently
> depends on that definition.
> 

I don't buy that as justification, if some userspace program uses it based 
on the false belief that it actually does what it says, it's probably 
better to break their build than perpetuating the lie that it's different 
than ~MAP_GROWSDOWN.


ia64: remove definition for MAP_GROWSUP

MAP_GROWSUP is unused.

Signed-off-by: David Rientjes <rientjes@google.com>
---
diff --git a/arch/ia64/include/asm/mman.h b/arch/ia64/include/asm/mman.h
--- a/arch/ia64/include/asm/mman.h
+++ b/arch/ia64/include/asm/mman.h
@@ -11,7 +11,6 @@
 #include <asm-generic/mman-common.h>
 
 #define MAP_GROWSDOWN	0x00100		/* stack-like segment */
-#define MAP_GROWSUP	0x00200		/* register stack-like segment */
 #define MAP_DENYWRITE	0x00800		/* ETXTBSY */
 #define MAP_EXECUTABLE	0x01000		/* mark it as an executable */
 #define MAP_LOCKED	0x02000		/* pages are locked */

Re: [PATCH] remove duplicate asm/mman.h files

[Arnd Bergmann]

On Monday 21 September 2009, David Rientjes wrote:
> > I tried not to change the ABI in any way in my patch, and there is
> > a theoretical possibility that some user space program on ia64 currently
> > depends on that definition.
> > 
> 
> I don't buy that as justification, if some userspace program uses it based 
> on the false belief that it actually does what it says, it's probably 
> better to break their build than perpetuating the lie that it's different 
> than ~MAP_GROWSDOWN.

It's more a matter of principle of my patches. I try to strictly separate
patches that move code around (like the one I sent) from those that
change contents (like yours, or the one before that adds MAP_STACK and
MAP_HUGETLB).

Removing a definition from an exported header file either requires
specific knowledge about why it is there to start with, or more
research on the topic than I wanted to do. For instance, a theoretical
program might have a helper function correctly doing

void *xmmap(void *addr, size_t length, int prot, int flags,
                  int fd, off_t offset)
{
	if (flags & MAP_GROWSUP) { /* MAP_GROWSUP is not supported */
		errno = -EINVAL;
		return MAP_FAILED;
	}

	return mmap(addr, length, prot, flags, fd, offset);
}

Of course, such a program would only work on ia64 currently, so
it should be safe to make ia64 behave like the other architectures
in this regard.

> ia64: remove definition for MAP_GROWSUP
> 
> MAP_GROWSUP is unused.
>
> Signed-off-by: David Rientjes <rientjes@google.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] remove duplicate asm/mman.h files

[Hugh Dickins]

On Mon, 21 Sep 2009, David Rientjes wrote:
> On Mon, 21 Sep 2009, Arnd Bergmann wrote:
> 
> > > > -#define MCL_CURRENT	1		/* lock all current mappings */
> > > > -#define MCL_FUTURE	2		/* lock all future mappings */
> > > > +#define MAP_GROWSUP	0x0200		/* register stack-like segment */
> > > >  
> > > >  #ifdef __KERNEL__
> > > >  #ifndef __ASSEMBLY__
> > > 
> > > ia64 doesn't use MAP_GROWSUP, so it's probably not necessary to carry it 
> > > along with your cleanup.
> > 
> > ia64 is the only architecture defining it, nobody uses it in the kernel.
> > If the ia64 maintainers want to remove it in a separate patch, that
> > would probably be a good idea.
> > 
> 
> I'll do it then.
> 
> > I tried not to change the ABI in any way in my patch, and there is
> > a theoretical possibility that some user space program on ia64 currently
> > depends on that definition.
> > 
> 
> I don't buy that as justification, if some userspace program uses it based 
> on the false belief that it actually does what it says, it's probably 
> better to break their build than perpetuating the lie that it's different 
> than ~MAP_GROWSDOWN.
> 
> 
> ia64: remove definition for MAP_GROWSUP
> 
> MAP_GROWSUP is unused.

Is it perhaps the case that some UNIX on ia64 does implement MAP_GROWSUP,
and these numbers in the Linux ia64 mman.h have been chosen to match that
reference implementation?  Tony will know.  But I wonder if you'd do
better at least to leave a MAP_GROWSUP comment on that line, so that
somebody doesn't go and reuse the empty slot later on.

Hugh

> 
> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
> diff --git a/arch/ia64/include/asm/mman.h b/arch/ia64/include/asm/mman.h
> --- a/arch/ia64/include/asm/mman.h
> +++ b/arch/ia64/include/asm/mman.h
> @@ -11,7 +11,6 @@
>  #include <asm-generic/mman-common.h>
>  
>  #define MAP_GROWSDOWN	0x00100		/* stack-like segment */
> -#define MAP_GROWSUP	0x00200		/* register stack-like segment */
>  #define MAP_DENYWRITE	0x00800		/* ETXTBSY */
>  #define MAP_EXECUTABLE	0x01000		/* mark it as an executable */
>  #define MAP_LOCKED	0x02000		/* pages are locked */

Re: [PATCH] remove duplicate asm/mman.h files

[David Rientjes]

On Mon, 21 Sep 2009, Hugh Dickins wrote:

> Is it perhaps the case that some UNIX on ia64 does implement MAP_GROWSUP,
> and these numbers in the Linux ia64 mman.h have been chosen to match that
> reference implementation?  Tony will know.  But I wonder if you'd do
> better at least to leave a MAP_GROWSUP comment on that line, so that
> somebody doesn't go and reuse the empty slot later on.
> 

Reserving the bit from future use by adding a comment may be helpful, but 
then let's do it for MAP_GROWSDOWN too.

RE: [PATCH] remove duplicate asm/mman.h files

[Luck, Tony]

>> Is it perhaps the case that some UNIX on ia64 does implement MAP_GROWSUP,
>> and these numbers in the Linux ia64 mman.h have been chosen to match that
>> reference implementation?  Tony will know.  But I wonder if you'd do
>> better at least to leave a MAP_GROWSUP comment on that line, so that
>> somebody doesn't go and reuse the empty slot later on.
>> 
>
> Reserving the bit from future use by adding a comment may be helpful, but 
> then let's do it for MAP_GROWSDOWN too.

Tony can only speculate because this bit has been in asm/mman.h
since before I started working on Linux (it is in the 2.4.0
version ... which is roughly when I started ... and long before
I was responsible for it).

Perhaps it was assumed that it would be useful?  Linux/ia64 does
use upwardly growing memory areas (the h/w register stack engine
saves "stack" registers to an area that grows upwards).

But since we have survived this long without it actually being
implemented, it may be true that we don't really need it after
all.

-Tony

RE: [PATCH] remove duplicate asm/mman.h files

[David Rientjes]

On Mon, 21 Sep 2009, Luck, Tony wrote:

> >> Is it perhaps the case that some UNIX on ia64 does implement MAP_GROWSUP,
> >> and these numbers in the Linux ia64 mman.h have been chosen to match that
> >> reference implementation?  Tony will know.  But I wonder if you'd do
> >> better at least to leave a MAP_GROWSUP comment on that line, so that
> >> somebody doesn't go and reuse the empty slot later on.
> >> 
> >
> > Reserving the bit from future use by adding a comment may be helpful, but 
> > then let's do it for MAP_GROWSDOWN too.
> 
> Tony can only speculate because this bit has been in asm/mman.h
> since before I started working on Linux (it is in the 2.4.0
> version ... which is roughly when I started ... and long before
> I was responsible for it).
> 
> Perhaps it was assumed that it would be useful?  Linux/ia64 does
> use upwardly growing memory areas (the h/w register stack engine
> saves "stack" registers to an area that grows upwards).
> 
> But since we have survived this long without it actually being
> implemented, it may be true that we don't really need it after
> all.
> 

glibc notes that both MAP_GROWSUP and MAP_GROWSDOWN are specific to Linux,
yet they don't functionally do anything.  While it may be true that 
there's no cost associated with keeping them around, I also think 
exporting such flags to userspace may give developers the belief that the 
implementation actually respects them when they're passed.

Ulrich wanted to do this last year but it appears to have been dropped.

Unless there's a convincing argument in the other direction, I don't see 
why they both can't just be removed and their bits reserved.

Re: [PATCH] remove duplicate asm/mman.h files

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

David Rientjes wrote:
> Ulrich wanted to do this last year but it appears to have been dropped.

I've mentioned that at that time, these flags cannot be used at all for
stacks.  And I don't know for what else it is useful.  They should
really be removed.

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkq4Ez0ACgkQ2ijCOnn/RHQNtACfX+y5pIQhDusikKiQwQ8nvGRN
cI8An0oThAXSwXRALt9598vbPbiVwEeJ
=sxfU
-----END PGP SIGNATURE-----