Skip to content
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

Missing requirements? #4

Closed
stlankes opened this issue Jul 23, 2023 · 7 comments
Closed

Missing requirements? #4

stlankes opened this issue Jul 23, 2023 · 7 comments

Comments

@stlankes
Copy link
Contributor

Coole project! I tested your crate in our kernel (see hermit-os/kernel#803). However, sometimes the call talc.init(arena.into()) reboot the kernel. Do you have some requirements for the arena?

@SFBdragon
Copy link
Owner

Thanks! Glad to contribute to hermitcore, at least indirectly. Although I must warn you that I'm very likely to make breaking API changes in the near future and bump to v2; although this shouldn't be tough to accommodate. (Should've began with v0.1, oops.)

This issue with the code in hermitcore is that after init, the Talc is invalidated if moved and will almost certainly cause problems. (this is documented in the init function, but maybe I should clarify it elsewhere too to make this harder to accidentally do? Alternatively, see additional discussion below.)

let arena = unsafe { core::slice::from_raw_parts_mut(heap_bottom, heap_size) };
let mut talc = Talc::new();
unsafe {
	talc.init(arena.into());
}
self.heap = Some(talc);

Could instead be:

self.heap = Some(Talc::new());
unsafe { self.heap.as_mut().unwrap().init(Span::from_ptr_size(heap_bottom, size)); }

As long as self doesn't get moved around this should be fine. Let me know if this works/causes further issues.

This requirement is due to Talc containing an array of bucket headers which are pointed at by pointers on the heap. I'm unsure whether sticking to this limitation is a good idea though (it makes allocation failure for small heaps more intuitive, but that's pretty much where the benefits end I suppose). Alternatively I put the bucket list headers in the heap which alleviates this requirement, and will probably be necessary to satisfy miri (talc#2). I've already ensured that Talc handles being unable to establish a heap due to insufficient size, so increasing that minimum size requirement behind the scenes wouldn't result in any changes (Talc will just refuse to make any memory allocatable if the heap is too small and any attempt to allocate will signal OOM).

Having typed this out, I think I'll prioritize making this change.

(P.S. There are unspecified requirements, the arena can't contain the null address, I'll be documenting that too.)

@stlankes
Copy link
Contributor Author

stlankes commented Jul 23, 2023

Hm, I followed your suggestions (see hermit-os/kernel#803), but it doesn't work. Do you have any idea, why it isn't working?

@stlankes
Copy link
Contributor Author

By the way, if you use the lock_api, everyone is able to replace spinlock with the preferred implementation.

@stlankes
Copy link
Contributor Author

I created PR #6 to show the usage of the lock_api.

@SFBdragon
Copy link
Owner

Hm, I followed your suggestions (see hermitcore/libhermit-rs#803), but it doesn't work. Do you have any idea, why it isn't working?

Not at all, sorry. Where can I find how to get rusty-hermit up and running on my machine? I assume I'm just loading a built binary in QEMU or somesuch? As long as you've got a kprint to serial IO in there or something to get debug info out, hopefully I'll figure it out :)

@stlankes
Copy link
Contributor Author

I found the bug. It is in "my" part of the kernel. I will fix it in the next few days.

@SFBdragon
Copy link
Owner

Congrats on tracking that down!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants