Re: [PATCH v4 7/7] rust: workqueue: add examples

[Boqun Feng]

On Wed, Oct 04, 2023 at 02:06:05PM +0300, Konstantin Shelekhin wrote:
> 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@xxxxxxxx> 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.

This is not a problem until nvmet actually uses/switches to Rust, right?
;-) We can certainly improve the API when a real user needs something.
Or you know someone is already working on this?

[...]
> 
> 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);
>     }

All of your suggestions make senses to me, but because we don't have
many users right now, it's actually hard to determine a "best" API. I
like what we have right now because it's explicit: people won't need to
learn much about procedure macros to understand how it works, and it
also provides better opportunities for people who's yet not familiar
with Rust to give some reviews. So starting with something relatively
simple and verbose may not be a bad idea ;-)

Again, I like your idea, we need to explore that direction, but one
dragon at a time ;-)

Regards,
Boqun