Deconflate "pagemap"

[nwf]

I'm certain there's something wrong with this PR, but I am not certain what, exactly.

I found it confusing that there were two things that called themselves PageMap and kept tripping over the difference when working on #41. So, in order to reduce my tripping rate and in hopes that I might help others do the same, I propose that we rename the alloc.h Pagemap thing a Slabmap. Not great, but at least different.

Having done that, it seemed to make sense to ensure that all accesses from the Allocator to the Pagemap were via the Slabmap. And then it became apparent that everything was static, in fact, and so some additional cleanups were possible.

I suspect there is something I'm not getting; perhaps a different, out-of-tree setting for SNMALLOC_DEFAULT_PAGEMAP breaks this? In any case, let's see what CI thinks about it, since it works for me on my desk...

[mjp41]

@davidchisnall I think this will not support one of your experiments, but there are no tests in CI for your use case. Could you look at this to see if it does?

[nwf]

Even if the whole patch series breaks some use case, I think I'd like the first patch, which is just renaming, to be considered in its own right, and ideally the second too, because, in the CHERI case, the slabmap differentiates between size and pointer returns while the pagemap just stores pointers with metadata shoved into the bottom bits.

[mjp41]

Understood. That makes sense. I wonder if "ChunkMap" would be a better name to match the paper. I was planning to use "chunk" in the codebase more, but I haven't got around to the refactor.

[nwf]

New patch series is the same as before with better commit messages and now using the name ChunkMap. Thanks for the suggestion; SlabMap wasn't sitting well. :)

[davidchisnall]

I have tried refactoring the compartmentalisation code to work with this, but I now don't believe it's possible. In particular, I need to be able to allocate Allocator instances that have a stateful pagemap so that each one can refer both to the global pagemap (implicitly) and to the copy of the statemap in the unprivileged code that it refers to (explicitly).

This is why we originally made all of these interfaces non-static and added the pagemap adaptor (the thing with per-allocator state that sits between the allocator and the pagemap and allows it to validate pagemap updates, send them to another sandbox, and so on).

[nwf]

@davidchisnall I don't think I understand what you mean by "the statemap" but perhaps I'm not meant to.

But... in any case, are the static accesses in external_address and alloc_size, which can only refer to the global pagemap and not the per-allocator state, OK?

Can the first and second commits land without breaking your use case, even if the other two can't?

[davidchisnall]

Those may be okay. In the other codebase, we have a clear distinction between three things:

• The page map (the data structure that maps from page to stuff).
• The superslab map, which should probably be called a chunk map, which maps from page to chunks.
• The page map adaptor, which provides an interface for the allocator to set and query page to allocation mappings.

The three layers each add different abstractions. In the default snmalloc config, the second and third of these are somewhat conflated (because the adaptor just forwards to the data structure), but we want to maintain this layering.

[nwf]

OK; since the layers may change the data being passed around, I think that hiding the page map proper from the Allocator is a good idea. If external_address and alloc_size need static access to the chunk map, then its accessors should be made static?

[mjp41]

Hi @nwf I spoke with @davidchisnall, This is my current understanding of how we should view it

• We have a generic data structure the page map.
• We have an single static instance of that generic data structure, the chunk map.
• We have a potentiall stateful wrapper of the "chunk map", the "chunk map provider".

The get routine on the "chunk map provider" can be static. In our current thinking, that does not need to take account of allocator local state, only in when updating the underlying chunk map.

I think with a few small changes, we can take this change.

[nwf]

Here's another stab that is effectively a better version of the first two commits of the old series. The interface to chunkmaps is prepared for non-static mutators, but the default implementation remains a 0-size class. Does this work for your case, @davidchisnall?

[davidchisnall]

I think it should. I'll take a look at applying it when I'm back in the office on Wednesday, but feel free to merge before then. I think it's a valuable cleanup and I'd rather fix any issues later than delay the merge.

[mjp41]

LGTM, one minor thing, not yours but while your changing it if you could replace the fixme it would be great.

[nwf]

@mjp41: done.

While in the neighborhood, I took the liberty of cherry-picking some of my earlier commentary additions as well; I think they're unobjectionable, but please advise.

[mjp41]

Clangformat is unhappy:

ninja clangformat

and then commit again! Thanks

[nwf]

Let's try that again.