[PATCH] PAGE_SIZE definition for MIPS XLP

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] PAGE_SIZE definition for MIPS XLP

Andrew Stubbs-5
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

xlp-page-size.patch (808 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Andreas Schwab
Andrew Stubbs <[hidden email]> 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, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Joseph Myers
In reply to this post by Andrew Stubbs-5
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
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Joseph Myers
In reply to this post by Andrew Stubbs-5
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
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Andrew Pinski-3
In reply to this post by Andrew Stubbs-5
On Mon, Nov 18, 2013 at 4:29 AM, Andrew Stubbs <[hidden email]> 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

removepagesize.diff.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Joseph Myers
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
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Andrew Stubbs-5
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Maciej W. Rozycki-4
In reply to this post by Andreas Schwab
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] PAGE_SIZE definition for MIPS XLP

Maciej W. Rozycki-4
In reply to this post by Andrew Pinski-3
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