LockIniter API concerns
#772
Comments
Thanks! I think I speak for everyone when I say we appreciate that.
This is not a concern, because kernel internal APIs are unstable, thus a
We want to minimize unsafe code in "leaf modules" (e.g. drivers) as much as possible, and we are aware there are a few suboptimal things at the moment, e.g. some were discussed with upstream Rust in November CTCFT. Of course, we welcome new ideas, input and contributors. However, this is not a "fatal flaw" at all, for two reasons: the main one being that this can be improved at any time in the future as mentioned above (and we do hope that happens for this and other APIs!), like any other internals in the kernel. The other reason is that unsafe code is not bad on its own: while we hope most modules/drivers can be written in fully safe Rust code and while unsafe code should not be used without good reason, unsafe code may still be needed in some cases -- that is fine, and it is a big improvement over C.
I disagree. Having
I appreciate that you are trying to warn us about using In other words, it is not that we are "happy" to have
There are several concerns at play here. We are not against proc-macros (in fact, we use them), but we do try to minimize their usage; and we would prefer language-level solutions wherever possible.
I disagree. There are things that should not be proc-macros -- at all -- and I hope Rust does not make the mistake of leaving everything up to a proc-macro. For instance, I hope
No need to apologize! |
That is relieving to hear, I was under the impression that if it were added to mainline, it would be a somewhat stable API. This is definitely changes some of my points.
Yes, if the API is unstable, then this is not a fatal flaw.
That is why I wanted to raise these concerns, if it is possible to avoid using
Minimizing the use of proc-macros is of course a good idea, I am not advocating for the use of proc-macros wherever possible. As I already said, if something can be done using a proc-macro, it becomes more unlikely that it is instead implemented as a language construct. This is because we do not want to clutter the rust language with so many features that it becomes difficult to know what features to use and how they interact with each other (C++ has shown us what happens in that case). There are of course situations where having a language-level solution to something that can be also done with a proc-macro is beneficial. I do not know if this is the case with pinned initialization. But the general workflow has been that a library would provide the needed feature for the time that the specifics of the language feature would be discussed. It also is a testing ground for what works and what does not. The problem with providing this as a language level solution is that I have no idea how it should look like. What would the usage of it look at the construction site of a pinned init type? How does it look like when you want to create a Could you tell me some of the other concerns, or are they mostly discussed in #302 ? |
|
Hey @y86-dev, thanks for bringing this up. We're very happy to have passionate people join the effort! I was writing a longer response earlier on but since Miguel already clarified a few points, I'm removing most of what I had already written. Just to add some more context: we are of course aware of the poor ergonomics here, and we talked about this in CTCFT (link to the specific part here) and in more detail in Rust Linz (link to the specific part here). One outcome of this was a discussion on how the process would look like on the rust side (link to the beginning here) for people/project (like us) to bring issues up but not necessarily solutions -- you're the experts after all. Anyway, since you ask about how a language solution could look like, here's a link to the part of the video where we talk about one possible way to go about this. I also stress in the end that we'd be happy with alternative solutions that address the current poor ergonomics (and aligns with our current goals of minimal external dependencies). |
Thanks for the resources.
I have thought a little bit about what that could look like. But the biggest problem that I found was that I have no idea how to generalize and combine this new feature with existing ones. While you have explicitly stated that you do not need much (just verify that every field was assigned to), rust should be designed in such a way that the features play nicely with each other (there are some exceptions, but I think we should avoid adding more). Before we add some difficult to interact with feature, we would like to explore some more of the solution space. Is |
|
I have been exploring if what you detailed in the talk can be still be done with a macro and I think it is feasible. It should even work without a proc-macro. Could you have a look at the examples at https://github.com/y86-dev/simple-safe-init ? I will try to explain it here, as I plan to change some things: At the center of the library is a macro called init! { expr => Type {
foo();
let a = 10 + 12;
.b = a;
.c = *b;
init_other(.d);
}}
It can take other parameters (and be generic, but you cannot pass generic args at the moment) and you are also allowed to bind the result (but at the moment the syntax needs to be different, due to ambiguity). Specifying pinned fieldsBecause we want good pin interaction, the macro should also work on types that are pinned. But then the problem of pin-projection arises, what fields inherit the pin invariant and what fields do not? pin_data! {
pub struct Foo {
#pin
a: u64,
b: u64,
}
}This macro should at the same time also provide pin-projection, similar to Limitations
I want to add more documentation and try to find some solutions for the ambiguity. I have not thought about enums, but maybe something similar can be done (you would just need to specify the variant first), but I have not tested that. |
|
This sounds very similar to my |
|
There are some similarities, but also some differences:
Example from your documentation: struct NeedPin {
// Must points to itself
address: *const NeedPin,
_pinned: PhantomPinned,
}
// pin-init:
impl NeedPin {
pub fn new() -> impl Init<Self, Infallible> {
init_from_closure(|mut this: PinUninit<'_, Self>| -> InitResult<'_, Self, Infallible> {
let v = this.get_mut().as_mut_ptr();
unsafe { *ptr::addr_of_mut!((*v).address) = v };
Ok(unsafe { this.init_ok() })
})
}
}
// simple-safe-init:
impl NeedPin {
pub fn init_need_pin(this: PinInitMe<'_, Self>) -> InitProof<()> {
let ptr = unsafe { this.as_mut_ptr() };
init! { this => Self {
.address = ptr;
._pinned = PhantomPinned;
}}
}
}There is also an argument to be made to make |
|
Also what was the main concern with pin-init? Was it the requirement of |
|
I haven't looked at all the details yet, but I really like the idea of using regular macros to initialise fields and then use a dead-code initialisation of the struct to have the compiler catch missing/duplicate fields for us. The fact that this approach doesn't require proc macros is great. @y86-dev if you can polish your solution, I see no reason for us not to use it. |
|
I have started integrating the library into the kernel code. This way I will discover anything that will need some more work. You can look at my current progress here: https://github.com/y86-dev/linux/tree/simple-init Disclaimer: I wanted to only evaluate what my library needs in order to be useful, so I just tried to get it to work as hastily as possible. I rewrote the parts that are in conflict with my design and did not think if their new implementation is a good idea. When you think that the library is ready, this should be done again with more API considerations. Is your At the moment I have discovered the following shortcomings/issues open for discussion:
I would greatly appreciate any feedback! Lastly here are some examples from the samples: rust_semaphore.rsold:mpl kernel::Module for RustSemaphore {
fn init(name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
pr_info!("Rust semaphore sample (init)\n");
let mut sema = Pin::from(UniqueRef::try_new(Semaphore {
// SAFETY: `condvar_init!` is called below.
changed: unsafe { CondVar::new() },
// SAFETY: `mutex_init!` is called below.
inner: unsafe {
Mutex::new(SemaphoreInner {
count: 0,
max_seen: 0,
})
},
})?);
// SAFETY: `changed` is pinned when `sema` is.
let pinned = unsafe { sema.as_mut().map_unchecked_mut(|s| &mut s.changed) };
condvar_init!(pinned, "Semaphore::changed");
// SAFETY: `inner` is pinned when `sema` is.
let pinned = unsafe { sema.as_mut().map_unchecked_mut(|s| &mut s.inner) };
mutex_init!(pinned, "Semaphore::inner");
Ok(Self {
_dev: Registration::new_pinned(fmt!("{name}"), sema.into())?,
})
}
}new:impl KernelModule for RustSemaphore {
fn init(name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
pr_info!("Rust semaphore sample (init)\n");
let sema = Pin::from(UniqueRef::try_new(MaybeUninit::<Semaphore>::uninit())?);
let sema = kernel::init! {sema => Semaphore {
kernel::init_with_lockdep!(.changed, "Semaphore::changed");
kernel::init_with_lockdep!(.inner, "Semaphore::inner", SemaphoreInner { count: 0, max_seen: 0 });
}};
Ok(Self {
_dev: Registration::new_pinned(fmt!("{name}"), sema.into())?,
})
}
}rust_miscdev.rsold:impl SharedState {
fn try_new() -> Result<Ref<Self>> {
let mut state = Pin::from(UniqueRef::try_new(Self {
// SAFETY: `condvar_init!` is called below.
state_changed: unsafe { CondVar::new() },
// SAFETY: `mutex_init!` is called below.
inner: unsafe { Mutex::new(SharedStateInner { token_count: 0 }) },
})?);
// SAFETY: `state_changed` is pinned when `state` is.
let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.state_changed) };
kernel::condvar_init!(pinned, "SharedState::state_changed");
// SAFETY: `inner` is pinned when `state` is.
let pinned = unsafe { state.as_mut().map_unchecked_mut(|s| &mut s.inner) };
kernel::mutex_init!(pinned, "SharedState::inner");
Ok(state.into())
}
}new:impl SharedState {
fn try_new() -> Result<Ref<Self>> {
let mut state = Pin::from(UniqueRef::try_new(core::mem::MaybeUninit::<Self>::uninit())?);
let mut state = kernel::init! {state => SharedState {
kernel::init_with_lockdep!(.state_changed, "SharedState::state_changed");
kernel::init_with_lockdep!(.inner, "SharedState::inner", SharedStateInner { token_count: 0 });
}};
Ok(state.into())
}
}rust_sync.rsold:impl kernel::Module for RustSync {
fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
pr_info!("Rust synchronisation primitives sample (init)\n");
// Test mutexes.
{
// SAFETY: `init` is called below.
let mut data = Pin::from(Box::try_new(unsafe { Mutex::new(0) })?);
mutex_init!(data.as_mut(), "RustSync::init::data1");
*data.lock() = 10;
pr_info!("Value: {}\n", *data.lock());
// SAFETY: `init` is called below.
let mut cv = Pin::from(Box::try_new(unsafe { CondVar::new() })?);
condvar_init!(cv.as_mut(), "RustSync::init::cv1");
{
let mut guard = data.lock();
while *guard != 10 {
let _ = cv.wait(&mut guard);
}
}
cv.notify_one();
cv.notify_all();
cv.free_waiters();
}
// Test static mutex + condvar.
*SAMPLE_MUTEX.lock() = 20;
{
let mut guard = SAMPLE_MUTEX.lock();
while *guard != 20 {
let _ = SAMPLE_CONDVAR.wait(&mut guard);
}
}
// Test spinlocks.
{
// SAFETY: `init` is called below.
let mut data = Pin::from(Box::try_new(unsafe { SpinLock::new(0) })?);
spinlock_init!(data.as_mut(), "RustSync::init::data2");
*data.lock() = 10;
pr_info!("Value: {}\n", *data.lock());
// SAFETY: `init` is called below.
let mut cv = Pin::from(Box::try_new(unsafe { CondVar::new() })?);
condvar_init!(cv.as_mut(), "RustSync::init::cv2");
{
let mut guard = data.lock();
while *guard != 10 {
let _ = cv.wait(&mut guard);
}
}
cv.notify_one();
cv.notify_all();
cv.free_waiters();
}
Ok(RustSync)
}
}new:impl KernelModule for RustSync {
fn init(_name: &'static CStr, _module: &'static ThisModule) -> Result<Self> {
pr_info!("Rust synchronisation primitives sample (init)\n");
// Test mutexes.
{
let data = Pin::from(Box::try_new(MaybeUninit::<Mutex<_>>::uninit())?);
let data = init_with_lockdep!(data, "RustSync::init::data1", 0);
*data.lock() = 10;
pr_info!("Value: {}\n", *data.lock());
let cv = Pin::from(Box::try_new(MaybeUninit::<CondVar>::uninit())?);
let cv = init_with_lockdep!(cv, "RustSync::init::cv1");
{
let mut guard = data.lock();
while *guard != 10 {
let _ = cv.wait(&mut guard);
}
}
cv.notify_one();
cv.notify_all();
cv.free_waiters();
}
// Test static mutex + condvar.
*SAMPLE_MUTEX.lock() = 20;
{
let mut guard = SAMPLE_MUTEX.lock();
while *guard != 20 {
let _ = SAMPLE_CONDVAR.wait(&mut guard);
}
}
// Test spinlocks.
{
let data = Pin::from(Box::try_new(MaybeUninit::<SpinLock<_>>::uninit())?);
let data = init_with_lockdep!(data, "RustSync::init::data2", 0);
*data.lock() = 10;
pr_info!("Value: {}\n", *data.lock());
let mut cv = Pin::from(Box::try_new(MaybeUninit::<CondVar>::uninit())?);
let mut cv = init_with_lockdep!(cv, "RustSync::init::cv2");
{
let mut guard = data.lock();
while *guard != 10 {
let _ = cv.wait(&mut guard);
}
}
cv.notify_one();
cv.notify_all();
cv.free_waiters();
}
Ok(RustSync)
}
} |
|
Thanks a lot for this!
(By the way, do you have an email address to contact you?) |
|
Yes, I forgot to set it to public, here it is: y86-dev@protonmail.com |
|
The last few weeks have been pretty busy for me, but I think I can work on this in the next weeks. |
|
I have now implemented most of the changes to the lock traits that I think are necessary. You can take a look at them here: https://github.com/y86-dev/linux/tree/simple-init-new While browsing some files searching for more places to apply the new API I also found some points of interest and questions:
Here are some locations where I have added compatibility with the new init API:
Let me know what you think about these changes! |
|
I have found a new way to do this, please take a look at https://github.com/y86-dev/simple-safe-init/tree/experiment and the integration itself: https://github.com/y86-dev/linux/tree/simple-init-experiment The library itself now only has 329 lines and uses a declarative macro. It still uses a struct initializer to verify that every field is initialized exactly once. |
|
Since https://lore.kernel.org/rust-for-linux/20230408122429.1103522-1-y86-dev@protonmail.com/ has been merged we now have a safe initialization API! |
Dear Rust-for-Linux team,
I want to raise some important concerns regarding some of the API design of this project.
If this is the wrong way to communicate this concern, then please direct me to right way of raising my .
My background
I am mainly a Rust developer and only have limited experience with the Linux code base. I have programmed in Rust extensively, so I probably have a rust-centric perspective on the project and might not know the constraints you are working under.
My view on this project
Let me first state this: I love this project and only wish it the best!
I personally use Linux and love the thought of integrating the safety of rust in the most often written parts of the code.
Yet, I am deeply frustrated with some of the choices made during API design. I feel like if these were pushed into mainline Linux code base hastily, it would become a legacy nightmare in the future, because you cannot remove a unsafe, already deprecated API, because a lot of existing code will depend on it. I want to avoid this future:
The LockIniter API
I am singling out the
LockIniterAPI, because that is all I managed to look at in the short time span. Locks are one of the most important primitives, so if their API has poor design that will have severe implications for all future code in the kernel.I think it is a fatal flaw that you are required to use
unsafewhen you want to create and initialize a lock. This issue has been previously raised in a more general form (#290). I want to shortly illustrate the issue using aMutexas an example (but other locks have the same problem, because they need to be pinned to be used), go over the suggested solution and its problems from the previous thread and my proposal to fix this.(The following code snippets have been copied from
samples/rust/rust_sync.rs)In the init function of the kernel module, a
Mutexis created inside of aBoxand then initialized.After this point no more unsafe is needed to use the mutex, which is good. However in order to call
Mutex::new, we need to uphold the invariant that we callMutex::init_lockbefore we use it. Because locks are a fundamental tool that will be used in a majority of modules, this creates manyunsafeblocks that could be avoided with better API design.This piece of code is in my eyes not really an improvement over the C code. Yes, there is a safety comment and a clear identification of an unsafe operation. But this has two problems: It annoys C programmers, because while they still program using their old ways, they now need to occasionally sprinkle in an
unsafeblock, which does not really seem beneficial. Rust devs are annoyed, because there exist nicer abstractions that could solve this in a way more idiomatic way.When we integrate Rust into the Linux codebase, we should use all advantages that the language can give us, like compiler-checked invariants and memory safety. But when we use unsafe in major places like this, we basically end up with C-style code in the Rust syntax.
Solution 1: Extra Allocations
Just store the lock in
Pin<Box<_>>(or other smart pointers) and only let the lock hand out those instances.But the additional allocation and indirection are obviously inefficient.
Solution 2: nbdd0121's Approach
While I have not extensively looked at their approach, I can say a few things:
While it seems like this approach works as intended, the discussion died down, probably because of the proc-macro integration issue.
Solution 3: My personal approach
In the short time that I had since I came across this project, I only partially managed to implemented my own solution. It is similar to nbdd0121's approach, as it also uses a proc-macro. See below for the discussion.
My approach centers around using transmutation of types to allow a separation of the creation of
MutexUninit<T>and its subsequent initialization to aMutex<T>, such that one is only able to initialize if they have access toPin<P> where P: DerefMut<MutexUninit<T>>.This allows the call site to look like this:
It leverages the type system to ensure that one cannot call
lockonMutexUninit. This way, forgetting to initialize the lock or attempting to initialize it twice results in a compilation error.Now, unsafe is only needed to create bottom level primitives that need to call unsafe functions themselves. At the declaration site (e.g. the cases where you want to create a struct containing multiple mutexes) you can simply use a proc-macro on the struct and mark the types that require initialization.
I created a crate as a proof-of-concept. If you want to take a look at it, I published it here. I am currently still working to improve it.
The main problem with this approach may be the usage of a proc-macro, similar to nbdd0121's solution. I'm a little late for the discussion, but I still have a few additional points to add:
synis very dependable, big parts of the rust ecosystem (including the rustc itself!) depend on it. There should be no problem using it, asbindgenis already integrated.Obviously, I do not think that proc-macros are the right tool for every problem, but this is definitely an area where a proc macro is the preferred and idiomatic way.
Summary
I apologize for the late interruption in your progress. I am very much aware that you probably cannot wait to integrate your progress into the mainline kernel and finally add support for Rust in Linux. And believe me, I am as excited as you are and I am very thankful for your work. I just wanted to raise my concerns over the API design and I hope you can understand why I am writing you here.
The text was updated successfully, but these errors were encountered: