Messages in this thread | | | Date | Tue, 3 Oct 2023 22:29:47 +0000 | Subject | Re: [PATCH v4 7/7] rust: workqueue: add examples | From | Alice Ryhl <> |
| |
On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin <k.shelekhin@ftml.net> wrote: > +//! #[pin_data] > +//! struct MyStruct { > +//! value: i32, > +//! #[pin] > +//! work: Work<MyStruct>, > +//! } > +//! > +//! impl_has_work! { > +//! impl HasWork<Self> for MyStruct { self.work } > +//! } > +//! > +//! impl MyStruct { > +//! fn new(value: i32) -> Result<Arc<Self>> { > +//! Arc::pin_init(pin_init!(MyStruct { > +//! value, > +//! work <- new_work!("MyStruct::work"), > +//! })) > +//! } > +//! } > +//! > +//! impl WorkItem for MyStruct { > +//! type Pointer = Arc<MyStruct>; > +//! > +//! fn run(this: Arc<MyStruct>) { > +//! pr_info!("The value is: {}", this.value); > +//! } > +//! } > +//! > +//! /// This method will enqueue the struct for execution on the system workqueue, where its value > +//! /// will be printed. > +//! fn print_later(val: Arc<MyStruct>) { > +//! let _ = workqueue::system().enqueue(val); > +//! } > > I understand that this is highly opionated, but is it possible to make > the initialization less verbose?
The short answer is yes. There are safe alternatives that are much less verbose. Unfortunately, those alternatives give up some of the features that this design has. Specifically, they give up the feature that allows you to embed the work_struct inside custom structs. I need to be able to embed the work_struct inside of custom structs, so I did not go that route.
There are also some parts of this (mainly `impl_has_work!`) that I am unhappy with. I would be happy to see a solution that doesn't need it, but I couldn't figure out how to avoid it.
> Because the C version looks much, much cleaner and easier to grasp: > > struct my_struct { > i32 value; > struct work_struct work; > }; > > void log_value(struct work_struct *work) > { > struct my_struct *s = container_of(work, struct my_struct, work); > pr_info("The value is: %d\n", s->value); > } > > void print_later(struct my_struct &s) > { > INIT_WORK(&s->work, log_value); > schedule_work(&s->work); > }
Although I think that a part of this is just whether you are familiar with Rust syntax, there is definitely some truth to this. Your code is a lot closer to the machine code of what actually happens here. Perhaps it would be interesting to see what you get if you just unsafely do exactly the same thing in Rust? It would look something like this:
struct MyStruct { value: i32, work: bindings::work_struct, }
unsafe fn log_value(work: *mut bindings::work_struct) { unsafe { let s = container_of!(work, MyStruct, work); pr_info!("The value is: {}", (*s).value); } }
unsafe fn print_later(s: *mut bindings::work_struct) { unsafe { bindings::INIT_WORK(&mut (*s).work, log_value); bindings::schedule_work(&mut (*s).work); } }
(I didn't try to compile this.)
The problem with this approach is that it uses unsafe in driver code, but the goal behind Rust abstractions is to isolate all of the related unsafe code. The idea being that drivers using the workqueue do not need any unsafe code to use it. This means that, assuming these workqueue abstractions are correct, no driver can accidentally cause memory unsafety by using the workqueue wrong.
The main difficult part of making this happen is the container_of operation. We need to somehow verify *at compile time* that the container_of in log_value really is given a pointer to the work field of a MyStruct. Other than the things that are just how Rust looks, most of the verbosity is necessary to make this compile-time check possible.
Another thing it does is handle proper transfer of ownership. In my original example, MyStruct is reference counted (due to the use of Arc), so the workqueue passes ownership of one refcount to the workqueue, which eventually passes the refcount to run. When `this` goes out of scope at the end of `run`, the refcount is decremented and the struct is freed if the refcount dropped to zero.
If you wanted to just have exclusive ownership of my_struct, you could do that by using Box instead of Arc. In either case, the ownership is correctly passed to run, and you cannot accidentally forget to free it at the end of log_value.
So, ultimately there's a tradeoff here. The code corresponds less directly to what the machine code will be. On the other hand, it will be *more* difficult to use incorrectly since incorrect uses will usually result in compilation errors. The claim of Rust is that this tradeoff is worth it.
Alice
| |