-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move OS_PAGE_SIZE to PAL, add Linux PowerPC w/ 64KiB pages #185
Conversation
7a1e1c9
to
9e42302
Compare
src/pal/pal_linux.h
Outdated
* Allow build-time customization of OS_PAGE_SIZE since Linux is apparently | ||
* a little less opinionated across its various ports than many other OSes. | ||
*/ | ||
static constexpr size_t OS_PAGE_SIZE = SNMALLOC_OS_PAGE_SIZE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to allow a global override and assert that it's at least equal to the OS's limit? We can always support a page size that's greater than the OS wants, just not one that's smaller.
I also wonder if we want to have a page size in the AAL and the PAL and take the max of the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do any architectures out in the wild not support 4K pages, at least in principle? AFAIK ppc64 does, it just happens that Debian/ppc64le builds their kernel with 64K pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SPARC supports only 8KiB pages (though they might have fixed that in later SPARC)? That said, I don't particularly want to support SPARC.
Why is the external override for Linux? I think that I'd prefer to structure this as:
- Add an
enum
for the architecture. - Make the AAL expose a constexpr field with the correct enum value.
- Make this a
std::conditional
keyed off that field. - Use
SNMALLOC_OS_PAGE_SIZE
in the global constant that we currently initialise with the version from the PAL. - Add a static assert that this global is a multiple of
PAL::OS_PAGE_SIZE
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, indeed, UltraSPARC sayeth:
The MMUs may support up to eight page sizes: 8 KBytes, 64 KBytes, 512 KBytes, 4 MBytes, 32 MBytes, 256 MBytes, 2 GBytes, and 16 GBytes.
When you say "Add an enum
for the architecture" do you mean something like
enum aal_page_size {
AAL_PAGE_SIZE_4K,
// ...
AAL_PAGE_SIZE_64K
};
or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant replace / augment the PLATFORM_IS_X86
and similar with something like:
enum Architecture
{
X86,
ARM,
PowerPC
};
And then be able to write something like this, rather than relying on #ifdef
s:
static constexpr size_t OS_PAGE_SIZE = AAL::Arch == Architecture::PowerPC ? 0x10000 : 0x1000;
Though, actually, it would make me even happier to have these in one of the helper headers:
constexpr unsigned long long int operator "" _KiB(unsigned long long int v)
{
return v * 1024;
}
constexpr unsigned long long int operator "" _MiB(unsigned long long int v)
{
return v * 1024 * 1024;
}
And then to write it as:
static constexpr size_t OS_PAGE_SIZE = AAL::Arch == Architecture::PowerPC ? 64_KiB : 4_KiB;
But that's not necessary for this PR unless you feel like it.
src/pal/pal_open_enclave.h
Outdated
@@ -19,6 +19,9 @@ namespace snmalloc | |||
* PAL supports. | |||
*/ | |||
static constexpr uint64_t pal_features = 0; | |||
|
|||
static constexpr size_t OS_PAGE_SIZE = 0x1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OE is somewhat interesting, because its back end doesn't actually care about pages. I wonder what happens if we use a smaller than 4KiB page here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, experimentally dropping it to 0x800
and re-running the two_alloc_types
suggests that things don't immediately fall over (that is, the tests pass, but I don't think that one test has phenomenal coverage of OE).
132e8f2
to
2537fdb
Compare
So I think some of this constraint was to ensure that the Medium allocations could be turned off independently, and that the slabs for small allocations could be turned off. I removed most of this code, so I think probably as long as SUPERSLAB_SIZE is a multiple of OS_PAGE_SIZE, then everything should work. I think we are only turning things on and off at the superslab granularity. |
*/ | ||
static inline void pause() | ||
{ | ||
__asm__ volatile("or 27,27,27"); // "yield" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reminding me why I hate PowerPC assembly so much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why there isn't a pseudoinstruction hint in LLVM, but so it goes. This is no worse than any other "idempotent operator's encoding becomes hint instead" that everybody and their brother loves, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more the complete lack of syntactic hint to let you know that 27 is a register number or a literal that I dislike. PowerPC assembly seems designed to discourage anyone from ever reading any, let alone writing some.
src/pal/pal_linux.h
Outdated
* Allow build-time customization of OS_PAGE_SIZE since Linux is apparently | ||
* a little less opinionated across its various ports than many other OSes. | ||
*/ | ||
static constexpr size_t OS_PAGE_SIZE = SNMALLOC_OS_PAGE_SIZE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SPARC supports only 8KiB pages (though they might have fixed that in later SPARC)? That said, I don't particularly want to support SPARC.
Why is the external override for Linux? I think that I'd prefer to structure this as:
- Add an
enum
for the architecture. - Make the AAL expose a constexpr field with the correct enum value.
- Make this a
std::conditional
keyed off that field. - Use
SNMALLOC_OS_PAGE_SIZE
in the global constant that we currently initialise with the version from the PAL. - Add a static assert that this global is a multiple of
PAL::OS_PAGE_SIZE
Does that make sense?
Unfortunately, untangling the dependencies here is harder than I thought it would be. We can't just have concluded the correct value of |
I see. I wonder if we want to just not bother with the ability to override the PAL then (you can always subclass a PAL and provide your own versions if you want to). Then, the POSIX PAL can just pull the value from its template parameter. |
5c5e204
to
04f888d
Compare
The CI failure seems to have been a timeout of |
With large pages (e.g. the 64K that Debian defaults to for ppc64), this is a bit much to ask. It's only not true for the bottom few medium size classes, tho', as all sizes above 256K are multiples of 64K with the current two mantissa bits size schedule.
Rebased to fix conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @davidchisnall, if your happy then I think we should merge.
At least the latter half is necessary for Debian/PowerPC64 support.
The first commit here should have no functional changes and just lay the groundwork. Admittedly, tho', it's selected out of a much larger mass of changes, so perhaps I've missed something. Let's see what CI says.
Should we add a check to
pal_posix
at startup thatOS_PAGE_SIZE
is divisible bysysconf(_SC_PAGESIZE)
?