Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri,  2 Jun 2023 09:37:41 +0000
In-Reply-To: <ZHkI8LXrqUOJ7G5E@boqun-archlinux>
Mime-Version: 1.0
References: <ZHkI8LXrqUOJ7G5E@boqun-archlinux>
Message-ID: <20230602093741.1040283-1-aliceryhl@google.com>
Subject: Re: [PATCH v2 5/8] rust: workqueue: add helper for defining
 work_struct fields
From:   Alice Ryhl <aliceryhl@google.com>
To:     boqun.feng@gmail.com
Cc:     alex.gaynor@gmail.com, aliceryhl@google.com,
        benno.lossin@proton.me, bjorn3_gh@protonmail.com, gary@garyguo.net,
        jiangshanlai@gmail.com, linux-kernel@vger.kernel.org,
        ojeda@kernel.org, patches@lists.linux.dev,
        rust-for-linux@vger.kernel.org, tj@kernel.org, wedsonaf@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Boqun Feng <boqun.feng@gmail.com> writes:
> On Thu, Jun 01, 2023 at 01:49:43PM +0000, Alice Ryhl wrote:
>> +/// Defines the method that should be called directly when a work item is executed.
>> +///
>> +/// Typically you would implement [`WorkItem`] instead. The `run` method on this trait will
>> +/// usually just perform the appropriate `container_of` translation and then call into the `run`
>> +/// method from the [`WorkItem`] trait.
>> +///
>> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
>> +///
>> +/// # Safety
>> +///
>> +/// Implementers must ensure that [`__enqueue`] uses a `work_struct` initialized with the [`run`]
>> +/// method of this trait as the function pointer.
>> +///
>> +/// [`__enqueue`]: RawWorkItem::__enqueue
>> +/// [`run`]: WorkItemPointer::run
>> +pub unsafe trait WorkItemPointer<const ID: u64>: RawWorkItem<ID> {
>> +    /// Run this work item.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// The provided `work_struct` pointer must originate from a previous call to `__enqueue` where
>> +    /// the `queue_work_on` closure returned true, and the pointer must still be valid.
>> +    unsafe extern "C" fn run(ptr: *mut bindings::work_struct);
>> +}
>> +
>> +/// Defines the method that should be called when this work item is executed.
>> +///
>> +/// This trait is used when the `work_struct` field is defined using the [`Work`] helper.
>> +pub trait WorkItem<const ID: u64 = 0> {
>> +    /// The pointer type that this struct is wrapped in. This will typically be `Arc<Self>` or
>> +    /// `Pin<Box<Self>>`.
>> +    type Pointer: WorkItemPointer<ID>;
> 
> This being an associate type makes me wonder how do we want to support
> the following (totally made-up by me, but I think it makes sense)?:
> 
> Say we have a struct
> 
> 	pub struct Foo {
> 		work: Work<Foo>,
> 		data: Data,
> 	}
> 
> 	impl Foo {
> 		pub fn do_sth(&self) {
> 			...
> 		}
> 	}
> 
> and we want to queue both Pin<Box<Foo>> and Arc<Foo> as work items, but
> the following doesn't work:
> 
> 	// Pin<Box<Foo>> can be queued.
> 	impl WorkItem for Foo {
> 		type Pointer = Pin<Box<Foo>>;
> 		fn run(ptr: Self::Pointer) {
> 			ptr.do_sth();
> 		}
> 	}
> 
> 	// Arc<Foo> can also be queued.
> 	impl WorkItem for Foo {
> 		type Pointer = Arc<Foo>;
> 		fn run(ptr: Self::Pointer) {
> 			ptr.do_sth();
> 		}
> 	}
> 
> Of course, we can use new type idiom, but that's not really great, and
> we may have more smart pointer types in the future.
> 
> Am I missing something here?

Basically, you're asking "is it possible to use the same `work_struct`
field for several different pointer types"?

When creating the function pointer to store in the `work_struct`, the
function pointer _must_ know what the pointer type is. Otherwise it
cannot call the right `WorkItem::run` method or perform the correct
`container_of` operation. (E.g. an `Arc` embeds a `refcount_t` before
the struct, but a `Box` does not.)

With an associated type there is no problem with that. Associated types
force you to make a choice, which means that the `work_struct` knows
what the pointer type is when you create it. Supporting what you suggest
means that we must be able to change the function pointer stored in the
`work_struct` after initializing it.

This is rather tricky because you can call `enqueue` from several
threads in parallel; just setting the function pointer before you call
`queue_work_on` would be a data race. I suppose you could do it by
implementing our own `queue_work_on` that sets the function pointer
_after_ successfully setting the `WORK_STRUCT_PENDING_BIT`, but I don't
think this patchset should do that.

Alice