From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B6F1611182 for ; Wed, 4 Oct 2023 11:06:10 +0000 (UTC) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id B76135C0376; Wed, 4 Oct 2023 07:06:09 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute3.internal (MEProxy); Wed, 04 Oct 2023 07:06:09 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ftml.net; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to; s=fm2; t= 1696417569; x=1696503969; bh=QmTK4qF8bJkFxhIME1kS98D3S1rwh87nee/ G/j+p6hE=; b=dNHvnA+FXEoYw28tPnYoekKUOl2jPEuaNs2UEODok7F+axlC2/m BKDU0d4R3IUa/5dyqfHaj11liKqH1DhIBA1Mwa1HmzR1K+wVVBGlhGtaCdwe9EnZ 9au0JYDa9f82kwz1oMMh4TJOOz9+9elN5YfTlrVXgKlhOeiSnCn5pAk9LMqj4cGb 2f4PzLFjfkrTh6yo+ZEDjXna2Kq9dzKsWsgOHrTP7VTwxyChgW/W4YtiBZPj1eRd 6axgv0EzMRrse4xlQaoqx+2227A8rSjRSdtcvzBdcLzPXqTbxQpP+UpRmFte2krF PS075JtWbJHqv1cXOV9162urDaqL9wCSTrA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t= 1696417569; x=1696503969; bh=QmTK4qF8bJkFxhIME1kS98D3S1rwh87nee/ G/j+p6hE=; b=MmaO5j+gl7OH+eAPMfy35YTsJQvw3OZUL7WV7xD+Wp8Lri9KRlL YpTC2zrvnHVCW8B0zEymmwk5/ABhLnKIFibPRm59OeSp1K8V9IST7ADOI7VZGCBd aS48Q8qtikX0zBUc4hy/hAij8txujpKzTIekQTaAqFVzrXsFZ5JO/EkGALmfEt0w QYiAKWJFJFfd/KEkJ3/aug10b+FLjGMdd3x/6vfwNrjTHsihpqwrWx9CEpKjZUPx Azzc/w53BJMfL4e2H5NBfJG+E0oZ7rkTqOgPTuaGbJdMEUmcy3JxNLpTZUnajB59 WnLGzZayYV+KBJ2ABQL23WDh2V/Z2iQ1aHg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrgedvgdefgecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpegggfgtfffkvefvhffuofhfjgesthhqredtredtjeenucfhrhhomhepfdfmohhn shhtrghnthhinhcuufhhvghlvghkhhhinhdfuceokhdrshhhvghlvghkhhhinhesfhhtmh hlrdhnvghtqeenucggtffrrghtthgvrhhnpeejffegkeeujefhfeettedtffefveeuteeg kedvjefhkeevkefgleffgedtgeduteenucevlhhushhtvghrufhiiigvpedtnecurfgrrh grmhepmhgrihhlfhhrohhmpehkrdhshhgvlhgvkhhhihhnsehfthhmlhdrnhgvth X-ME-Proxy: Feedback-ID: ib7794740:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 4 Oct 2023 07:06:06 -0400 (EDT) Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 04 Oct 2023 14:06:05 +0300 Message-Id: Cc: , , , , , , , , , , , , , To: "Alice Ryhl" From: "Konstantin Shelekhin" Subject: Re: [PATCH v4 7/7] rust: workqueue: add examples X-Mailer: aerc 0.15.2.r131.ga5d6a70 References: <20231003222947.374039-1-aliceryhl@google.com> In-Reply-To: <20231003222947.374039-1-aliceryhl@google.com> On Wed Oct 4, 2023 at 1:29 AM MSK, Alice Ryhl wrote: > On Tue, Oct 3, 2023 at 10:13PM Konstantin Shelekhin wrote: > > +//! #[pin_data] > > +//! struct MyStruct { > > +//! value: i32, > > +//! #[pin] > > +//! work: Work, > > +//! } > > +//! > > +//! impl_has_work! { > > +//! impl HasWork for MyStruct { self.work } > > +//! } > > +//! > > +//! impl MyStruct { > > +//! fn new(value: i32) -> Result> { > > +//! Arc::pin_init(pin_init!(MyStruct { > > +//! value, > > +//! work <- new_work!("MyStruct::work"), > > +//! })) > > +//! } > > +//! } > > +//! > > +//! impl WorkItem for MyStruct { > > +//! type Pointer =3D Arc; > > +//! > > +//! fn run(this: Arc) { > > +//! pr_info!("The value is: {}", this.value); > > +//! } > > +//! } > > +//! > > +//! /// This method will enqueue the struct for execution on the syste= m workqueue, where its value > > +//! /// will be printed. > > +//! fn print_later(val: Arc) { > > +//! let _ =3D 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; > > }; > >=20 > > void log_value(struct work_struct *work) > > { > > struct my_struct *s =3D container_of(work, struct my_struct, wo= rk); > > pr_info("The value is: %d\n", s->value); > > } > >=20 > > 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 =3D 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 =3D log_value)] work: Work, } fn log_value(s: Arc) { pr_info!("The value is: {}", s.value); } fn print_later(s: Arc) { workqueue::system().enqueue(s); }