public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] PAGE_SIZE definition for MIPS XLP
@ 2013-11-18 12:51 Andrew Stubbs
  2013-11-18 13:42 ` Andreas Schwab
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Andrew Stubbs @ 2013-11-18 12:51 UTC (permalink / raw)
  To: libc-ports

[-- Attachment #1: Type: text/plain, Size: 1269 bytes --]

MIPS' sys/user.h currently has a constant definition for PAGE_SIZE, and 
the other related settings. This is not appropriate for XLP (and other 
MIPS?) where the actual page size is a kernel configuration option.

Apart from the general principle of not having incorrect definitions, 
the actual problem that needs to be solved is in 
sysdeps/unix/sysv/linux/ifaddrs.c in which PAGE_SIZE is used by 
preference as an optimization. Most of the other possible use cases 
prefer to call __getpagesize or use sysconf, and so are unaffected.

Clearly, keeping the constant definition is desirable on at least some 
MIPS variants, in order to keep the optimization, but not for XLP.

The attached patch makes the definition conditional, rather than 
removing it completely. It's not clear to me whether the HOST_* 
definitions are similarly affected, but other platforms that do not 
define PAGE_SIZE also choose not to define those, so I've extended the 
ifndef similarly.

I this OK to commit? Should it be solved a different way?

Testcase tst-limits does check PAGE_SIZE matches, if defined, but not in 
this case because that test case does not include sys/user.h. Should I 
create a new test case for this, or include that header in the existing 
test?

Thanks

Andrew

[-- Attachment #2: xlp-page-size.patch --]
[-- Type: text/x-patch, Size: 783 bytes --]

2013-11-18  Andrew Stubbs  <ams@codesourcery.com>

	ports/
	* sysdeps/unix/sysv/linux/mips/sys/user.h: Don't define page
	characteristics on XLP.

diff --git a/ports/sysdeps/unix/sysv/linux/mips/sys/user.h b/ports/sysdeps/unix/sysv/linux/mips/sys/user.h
index 37fc568..c0d1505 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/sys/user.h
+++ b/ports/sysdeps/unix/sysv/linux/mips/sys/user.h
@@ -206,6 +206,7 @@ struct user {
 
 #endif
 
+#ifndef _MIPS_ARCH_XLP
 #define PAGE_SHIFT		12
 #define PAGE_SIZE		(1UL << PAGE_SHIFT)
 #define PAGE_MASK		(~(PAGE_SIZE-1))
@@ -214,5 +215,6 @@ struct user {
 #define HOST_TEXT_START_ADDR	(u.start_code)
 #define HOST_DATA_START_ADDR	(u.start_data)
 #define HOST_STACK_END_ADDR	(u.start_stack + u.u_ssize * NBPG)
+#endif
 
 #endif	/* _SYS_USER_H */

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-18 12:51 [PATCH] PAGE_SIZE definition for MIPS XLP Andrew Stubbs
@ 2013-11-18 13:42 ` Andreas Schwab
  2013-11-19 20:56   ` Maciej W. Rozycki
  2013-11-18 13:45 ` Joseph S. Myers
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Andreas Schwab @ 2013-11-18 13:42 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: libc-ports

Andrew Stubbs <ams@codesourcery.com> writes:

> The attached patch makes the definition conditional, rather than removing
> it completely. It's not clear to me whether the HOST_* definitions are
> similarly affected, but other platforms that do not define PAGE_SIZE also
> choose not to define those, so I've extended the ifndef similarly.

These definitions are used by gdb for trad-core support.  Does MIPS
support trad-core?

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-18 12:51 [PATCH] PAGE_SIZE definition for MIPS XLP Andrew Stubbs
  2013-11-18 13:42 ` Andreas Schwab
@ 2013-11-18 13:45 ` Joseph S. Myers
  2013-11-18 18:21 ` Joseph S. Myers
  2013-11-19  3:28 ` Andrew Pinski
  3 siblings, 0 replies; 9+ messages in thread
From: Joseph S. Myers @ 2013-11-18 13:45 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: libc-ports

On Mon, 18 Nov 2013, Andrew Stubbs wrote:

> The attached patch makes the definition conditional, rather than removing it
> completely. It's not clear to me whether the HOST_* definitions are similarly
> affected, but other platforms that do not define PAGE_SIZE also choose not to
> define those, so I've extended the ifndef similarly.
> 
> I this OK to commit? Should it be solved a different way?

This should be raised on libc-alpha, not libc-ports, as it's a generic 
issue regarding what the glibc API is for PAGE_SIZE being provided or not 
provided in sys/user.h.

It's definitely wrong to test _MIPS_ARCH_XLP like that - glibc built for 
generic MIPS should work on all MIPS variants supporting all the 
instructions used in that glibc binary, so the fact that MIPS has variable 
page sizes means it's always wrong for MIPS glibc to embed a page size 
assumption in the binaries.  The question is whether PAGE_SIZE must only 
be defined when it is the kernel page size, or whether it should also be 
defined in some cases when it's some form of ABI page size even when that 
differs from the kernel page size.  I also see no rationale given for 
making any of the other macros conditional.

At least MicroBlaze also has variable kernel page size but defines 
PAGE_SIZE in sys/user.h.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-18 12:51 [PATCH] PAGE_SIZE definition for MIPS XLP Andrew Stubbs
  2013-11-18 13:42 ` Andreas Schwab
  2013-11-18 13:45 ` Joseph S. Myers
@ 2013-11-18 18:21 ` Joseph S. Myers
  2013-11-19  3:28 ` Andrew Pinski
  3 siblings, 0 replies; 9+ messages in thread
From: Joseph S. Myers @ 2013-11-18 18:21 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: libc-ports

Also, please make sure there is a bug open in glibc Bugzilla for the 
user-visible problem this is meant to fix (mentioning all affected 
architectures, or separate bugs for each architecture if you wish), if 
there isn't already.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-18 12:51 [PATCH] PAGE_SIZE definition for MIPS XLP Andrew Stubbs
                   ` (2 preceding siblings ...)
  2013-11-18 18:21 ` Joseph S. Myers
@ 2013-11-19  3:28 ` Andrew Pinski
  2013-11-19 14:57   ` Joseph S. Myers
  2013-11-20 19:57   ` Maciej W. Rozycki
  3 siblings, 2 replies; 9+ messages in thread
From: Andrew Pinski @ 2013-11-19  3:28 UTC (permalink / raw)
  To: Andrew Stubbs; +Cc: libc-ports

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On Mon, Nov 18, 2013 at 4:29 AM, Andrew Stubbs <ams@codesourcery.com> wrote:
> MIPS' sys/user.h currently has a constant definition for PAGE_SIZE, and the
> other related settings. This is not appropriate for XLP (and other MIPS?)
> where the actual page size is a kernel configuration option.

The whole Octeon series of MIPS64 processors also supports other
PAGE_SIZEs.  Also the generic MIPS glibc should support other page
sizes too.

>
> Apart from the general principle of not having incorrect definitions, the
> actual problem that needs to be solved is in
> sysdeps/unix/sysv/linux/ifaddrs.c in which PAGE_SIZE is used by preference
> as an optimization. Most of the other possible use cases prefer to call
> __getpagesize or use sysconf, and so are unaffected.
>
> Clearly, keeping the constant definition is desirable on at least some MIPS
> variants, in order to keep the optimization, but not for XLP.

No it is not desirable if you want a generic glibc which works on all MIPS64.

>
> The attached patch makes the definition conditional, rather than removing it
> completely. It's not clear to me whether the HOST_* definitions are
> similarly affected, but other platforms that do not define PAGE_SIZE also
> choose not to define those, so I've extended the ifndef similarly.
>
> I this OK to commit? Should it be solved a different way?

I think my attached patch is better way of fixing this issue which
just deletes them rather than special casing them.

Thanks,
Andrew Pinski

>
> Testcase tst-limits does check PAGE_SIZE matches, if defined, but not in
> this case because that test case does not include sys/user.h. Should I
> create a new test case for this, or include that header in the existing
> test?
>
> Thanks
>
> Andrew

[-- Attachment #2: removepagesize.diff.txt --]
[-- Type: text/plain, Size: 1019 bytes --]

commit 20364b09f52a6cb93188752b825b65fbad82e4d7
Author: Andrew Pinski <apinski@cavium.com>
Date:   Wed Jul 11 19:28:58 2012 -0700

    [PATCH 12/19] 2012-07-11  Andrew Pinski  <apinski@cavium.com>
    
    	Bug #2964
    	* ports/sysdeps/unix/sysv/linux/mips/sys/user.h (PAGE_SHIFT): Delete.
    	(PAGE_SIZE): Delete.
    	(PAGE_MASK): Delete.
    	(NBPG): Delete.
    	(HOST_STACK_END_ADDR): Delete.

diff --git a/ports/sysdeps/unix/sysv/linux/mips/sys/user.h b/ports/sysdeps/unix/sysv/linux/mips/sys/user.h
index 950cad7..6e83b80 100644
--- a/ports/sysdeps/unix/sysv/linux/mips/sys/user.h
+++ b/ports/sysdeps/unix/sysv/linux/mips/sys/user.h
@@ -206,13 +206,8 @@ struct user {
 
 #endif
 
-#define PAGE_SHIFT		12
-#define PAGE_SIZE		(1UL << PAGE_SHIFT)
-#define PAGE_MASK		(~(PAGE_SIZE-1))
-#define NBPG			PAGE_SIZE
 #define UPAGES			1
 #define HOST_TEXT_START_ADDR	(u.start_code)
 #define HOST_DATA_START_ADDR	(u.start_data)
-#define HOST_STACK_END_ADDR	(u.start_stack + u.u_ssize * NBPG)
 
 #endif	/* _SYS_USER_H */

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-19  3:28 ` Andrew Pinski
@ 2013-11-19 14:57   ` Joseph S. Myers
  2013-11-19 20:19     ` Andrew Stubbs
  2013-11-20 19:57   ` Maciej W. Rozycki
  1 sibling, 1 reply; 9+ messages in thread
From: Joseph S. Myers @ 2013-11-19 14:57 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: Andrew Stubbs, libc-ports

On Mon, 18 Nov 2013, Andrew Pinski wrote:

> I think my attached patch is better way of fixing this issue which
> just deletes them rather than special casing them.

Deleting (for both MIPS and MicroBlaze) would indeed be better if the API 
for these symbols is that they should only be defined if the kernel page 
size is constant.

Someone still needs to look into the API for all these symbols and write a 
proper explanation of how they are used (outside of glibc) and what 
requirements apply to them.  It's not clear whether it's right to remove 
the whole block from PAGE_SHIFT to HOST_STACK_END_ADDR, or only a subset 
that directly depend on the page size; that may depend on whether the 
other symbols are ever used in a context not also depending on PAGE_SIZE 
(are they only for BFD's trad-core?).  Given such an explanation, we can 
better judge a removal patch.  I don't want "this causes problems, so 
remove it"; I want "this is incorrect because (explanation of the 
interface), so removing it is correct".

Note that IA64 confuses things by defining a subset of the macros, 
including defining NBPG to PAGE_SIZE despite that header not defining 
PAGE_SIZE.

The patch does of course need a proper bug number from glibc Bugzilla (as 
I noted, either a bug needs filing for all affected architectures, or 
separate bugs for each architecture, in accordance with glibc practice 
that if fixing a bug that was user-visible in a release then you also file 
it in Bugzilla to make searches of fixed bugs more useful) and the path in 
the ChangeLog entry given without the initial ports/, as appropriate for 
ChangeLog.mips.

-- 
Joseph S. Myers
joseph@codesourcery.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-19 14:57   ` Joseph S. Myers
@ 2013-11-19 20:19     ` Andrew Stubbs
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Stubbs @ 2013-11-19 20:19 UTC (permalink / raw)
  To: Joseph S. Myers, Andrew Pinski; +Cc: Andrew Stubbs, libc-ports

On 19/11/13 13:50, Joseph S. Myers wrote:
> The patch does of course need a proper bug number from glibc Bugzilla

Done:

https://sourceware.org/bugzilla/show_bug.cgi?id=16191

Andrew

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-18 13:42 ` Andreas Schwab
@ 2013-11-19 20:56   ` Maciej W. Rozycki
  0 siblings, 0 replies; 9+ messages in thread
From: Maciej W. Rozycki @ 2013-11-19 20:56 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Andrew Stubbs, libc-ports

On Mon, 18 Nov 2013, Andreas Schwab wrote:

> > The attached patch makes the definition conditional, rather than removing
> > it completely. It's not clear to me whether the HOST_* definitions are
> > similarly affected, but other platforms that do not define PAGE_SIZE also
> > choose not to define those, so I've extended the ifndef similarly.
> 
> These definitions are used by gdb for trad-core support.  Does MIPS
> support trad-core?

 I don't think it does, not at least on Linux where ELF has been ever used 
only (and ECOFF compatibility ABI never implemented), but as a side note 
trad-core has this:

#ifndef NBPG
# define NBPG getpagesize()
#endif

so it seems to be broken anyway for non-native cases.  I'd expect the page 
size in a core file to match that used by the kernel while the process was 
still alive, so it would have to be figured out from the core binary being 
handled somehow if at all possible; otherwise a user-supplied parameter.

  Maciej

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PAGE_SIZE definition for MIPS XLP
  2013-11-19  3:28 ` Andrew Pinski
  2013-11-19 14:57   ` Joseph S. Myers
@ 2013-11-20 19:57   ` Maciej W. Rozycki
  1 sibling, 0 replies; 9+ messages in thread
From: Maciej W. Rozycki @ 2013-11-20 19:57 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: Andrew Stubbs, libc-ports

On Tue, 19 Nov 2013, Andrew Pinski wrote:

> > MIPS' sys/user.h currently has a constant definition for PAGE_SIZE, and the
> > other related settings. This is not appropriate for XLP (and other MIPS?)
> > where the actual page size is a kernel configuration option.
> 
> The whole Octeon series of MIPS64 processors also supports other
> PAGE_SIZEs.  Also the generic MIPS glibc should support other page
> sizes too.

 Virtually all MIPS processors that have a TLB MMU, except a few old 
oddballs, support a configurable page size (on a page-by-page basis, but 
Linux sets the size globally).  The exceptions are the R2000/R3000 MIPS I 
ISA CPUs and their variations (fixed 4kB page size) and the R6000 MIPS II 
ISA CPU (fixed 16kB page size).  The latter hardly ever seen anywhere 
(being a costly ECL discrete CPU chip set) and never supported by Linux.  
Therefore there's no point ever in hardcoding any particular page size for 
the MIPS port.

 Of the sizes offered by standard hardware, ranging from 1kB up to 256TB 
at the every other power of 2, Linux chose to support 4kB, 16kB and 64kB 
pages.

> > Apart from the general principle of not having incorrect definitions, the
> > actual problem that needs to be solved is in
> > sysdeps/unix/sysv/linux/ifaddrs.c in which PAGE_SIZE is used by preference
> > as an optimization. Most of the other possible use cases prefer to call
> > __getpagesize or use sysconf, and so are unaffected.

 The case of ifaddrs.c seems ill-conceived to me.  If such a kind of 
optimisation is desired, then I think on targets that have a fixed page 
size getpagesize should simply be implemented as a static inline function 
so that GCC can reduce any calls to an instantiation of the constant 
returned.

  Maciej

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-11-19 20:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-18 12:51 [PATCH] PAGE_SIZE definition for MIPS XLP Andrew Stubbs
2013-11-18 13:42 ` Andreas Schwab
2013-11-19 20:56   ` Maciej W. Rozycki
2013-11-18 13:45 ` Joseph S. Myers
2013-11-18 18:21 ` Joseph S. Myers
2013-11-19  3:28 ` Andrew Pinski
2013-11-19 14:57   ` Joseph S. Myers
2013-11-19 20:19     ` Andrew Stubbs
2013-11-20 19:57   ` Maciej W. Rozycki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).