Message-Id: <CVZLU74VWMKA.GQXYH7WUNPS4@pogg>
Date:   Wed, 04 Oct 2023 14:06:05 +0300
From:   "Konstantin Shelekhin" <k.shelekhin@...l.net>
To:     "Alice Ryhl" <aliceryhl@...gle.com>
Cc:     <alex.gaynor@...il.com>, <benno.lossin@...ton.me>,
        <bjorn3_gh@...tonmail.com>, <boqun.feng@...il.com>,
        <gary@...yguo.net>, <jiangshanlai@...il.com>,
        <linux-kernel@...r.kernel.org>, <nmi@...aspace.dk>,
        <ojeda@...nel.org>, <patches@...ts.linux.dev>,
        <rust-for-linux@...r.kernel.org>, <tj@...nel.org>,
        <wedsonaf@...il.com>, <yakoyoku@...il.com>
Subject: Re: [PATCH v4 7/7] rust: workqueue: add examples

On Wed Oct 4, 2023 at 1:29 AM MSK, Alice Ryhl wrote:
> On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin <k.shelekhin@...l.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.

My concern with the approach of using traits instead of calling an
initialization function is that a some of existing code uses the
following pattern:

    static void nvmet_file_submit_buffered_io(struct nvmet_req *req)
    {
            INIT_WORK(&req->f.work, nvmet_file_buffered_io_work);
            queue_work(buffered_io_wq, &req->f.work);
    }

    static void nvmet_file_execute_flush(struct nvmet_req *req)
    {
            if (!nvmet_check_transfer_len(req, 0))
                    return;
            INIT_WORK(&req->f.work, nvmet_file_flush_work);
            queue_work(nvmet_wq, &req->f.work);
    }

    static void nvmet_file_execute_dsm(struct nvmet_req *req)
    {
            if (!nvmet_check_data_len_lte(req, nvmet_dsm_len(req)))
                    return;
            INIT_WORK(&req->f.work, nvmet_file_dsm_work);
            queue_work(nvmet_wq, &req->f.work);
    }

As you can see a single work struct is used here, and dispatching
happens beforehands. While it's possible to do the dispatching later in
run(), it's IMO cleaner to do this earlier.

> 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.

I get where all this coming from, I just really dislike the idea to
write all this code every time I need to pass something down the
workqueue. Maybe it's possible to hide most of the boilerplate behind a
derive.

Something like this, for example:

    #[pin_data, derive(WorkContainer)]
    struct MyStruct {
        value: i32,
        #[pin, work(fn = log_value)]
        work: Work,
    }

    fn log_value(s: Arc<MyStruct>) {
        pr_info!("The value is: {}", s.value);
    }

    fn print_later(s: Arc<MyStruct>) {
        workqueue::system().enqueue(s);
    }

Powered by blists - more mailing lists