google / zerocopy

https://discord.gg/MAvWH2R6zk
Apache License 2.0
1.61k stars 105 forks source link

Add support for allocating statically-aligned and dynamically-sized memory #1750

Open sutes-work opened 2 months ago

sutes-work commented 2 months ago

See also: #885, #249, #280.

What is the name of your project?

Fuchsia

Please provide a link to your project (GitHub repository, crates.io page, etc).

https://cs.opensource.google/fuchsia/fuchsia/+/main:src/storage/fvm/src/main.rs;drc=b04c5108ca31a3a8880119d06dad7e857c2a9b12;l=1598

What features would you like from zerocopy?

We have a need to allocate memory that has the same alignment as some other type T (that has the zerocopy traits) but is larger than T (it's used for storage so it needs to be rounded to block sizes). Once that's memory as been allocated, ideally it would be possible to use infallible zerocopy methods since the alignment (it's aligned) and size (it's big enough) should be known to be valid.

joshlf commented 1 month ago

One thing I don't understand from the linked code:

let mut buffer = AlignedMem::<Header>::new(
    (BLOCK_SIZE + self.header.vpartition_table_size) as usize
        + self.header.allocation_size()?,
);

What role does vpartition_table_size play? Since you're allocating BLOCK_SIZE + self.header.vpartition_table_size bytes, new will attempt to construct a new Layout internally, and if BLOCK_SIZE + self.header.vpartition_table_size is not a multiple of align_of::<Header>(), that will fail, and the following .unwrap() will panic:

alloc::Layout::from_size_align(size, std::mem::align_of::<T>()).unwrap(),

Is it guaranteed somehow that this won't happen?

joshlf commented 1 month ago

Okay, I whipped up something that does roughly what your code does, although it doesn't address the vpartition_table_size issue. Here it is on the Rust Playground. Note that the Playground uses zerocopy 0.7, so some of the trait names are different (IntoBytes -> AsBytes, FromZeros -> FromZeroes, and KnownLayout/Immutable don't exist).

Does that do roughly what you're hoping for?

sutes-work commented 1 month ago

Is it guaranteed somehow that this won't happen?

Yes. BLOCK_SIZE and vpartition_table_size will always be a power of 2 (as the alignment must be) and >= 512 (although your comment has reminded me to check this).

Does that do roughly what you're hoping for?

Not quite. The size needs to be variable. vpartition_table_size is not known at compilation time.

joshlf commented 1 month ago

Is it guaranteed somehow that this won't happen?

Yes. BLOCK_SIZE and vpartition_table_size will always be a power of 2 (as the alignment must be) and >= 512 (although your comment has reminded me to check this).

Where is vpartition_table_size actually set? In this file, I only see it parsed from external memory, which might have any contents.

sutes-work commented 1 month ago

Yeah, I have a TODO to add checks for this: https://cs.opensource.google/fuchsia/fuchsia/+/main:src/storage/fvm/src/main.rs;drc=b04c5108ca31a3a8880119d06dad7e857c2a9b12;l=206.

This code is under active development; nothing outside of tests uses it yet.

joshlf commented 1 month ago

Okay gotcha.

The dynamic size does make this difficult to support. I can see us supporting it at some point in the future, but the combination of statically-bounded alignment plus dynamic sizing makes things tricky. I'll follow up here if we end up adding features that could make this easier.