Skip to content

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

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

Open
sutes-work opened this issue Sep 24, 2024 · 6 comments
Labels
customer-request Documents customer requests.

Comments

@sutes-work
Copy link

sutes-work commented Sep 24, 2024

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.

@sutes-work sutes-work added the customer-request Documents customer requests. label Sep 24, 2024
@joshlf
Copy link
Member

joshlf commented Sep 24, 2024

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
Copy link
Member

joshlf commented Sep 24, 2024

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
Copy link
Author

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
Copy link
Member

joshlf commented Sep 24, 2024

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
Copy link
Author

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
Copy link
Member

joshlf commented Sep 24, 2024

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.

@joshlf joshlf changed the title Add support for allocating aligned memory Add support for allocating statically-aligned and dynamically-sized memory Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-request Documents customer requests.
Projects
None yet
Development

No branches or pull requests

2 participants