From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0898EC7EE24 for ; Fri, 2 Jun 2023 09:37:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234591AbjFBJht (ORCPT ); Fri, 2 Jun 2023 05:37:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51172 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234361AbjFBJhs (ORCPT ); Fri, 2 Jun 2023 05:37:48 -0400 Received: from mail-ej1-x64a.google.com (mail-ej1-x64a.google.com [IPv6:2a00:1450:4864:20::64a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0D5BBCE for ; Fri, 2 Jun 2023 02:37:45 -0700 (PDT) Received: by mail-ej1-x64a.google.com with SMTP id a640c23a62f3a-970e0152da7so139825866b.2 for ; Fri, 02 Jun 2023 02:37:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1685698663; x=1688290663; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=g6hKGnk7+yrdCqGxFZXtFZSCtr3Hs69nhywa4vN9dQk=; b=k++9SNiv/OO3O33g9/9eTroGH9b5hGlPNSi00zMI+3lKBu4RkFuIC1EcEe7KD/jbaz w7M2YFlWaueUEZ3ZBRhBGEljk4CX6XobHPYEr/cMV88eHC8OdecDaOMfZJuBeqInAwD+ AS49oX1gWO0jWTyaCmjdNL7X0Yk+k7i5creGBmVCLkabQjlsHTPe5sA28usbmDCXYCRJ 4ZptKGmHboTtMeV00P4JBm95Ws+ym2erT/xlVh4nZgISW78geGc+StYhiNHOeV4sSw4e PAkVnBlceCInTUdm4jIK/4AJvO4dTSuhgWJSM/z4SZ2w33u+uzuwv+yyv44kgQKOdQGn uWYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685698663; x=1688290663; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=g6hKGnk7+yrdCqGxFZXtFZSCtr3Hs69nhywa4vN9dQk=; b=J8lbhNUHwtU0lRheSWQIrU93w6/obXFsDsMs04kXVRVcu5hmwm+C+6JlOmNtJxOm8q cJ23PbsYw6LjHKH35Hn4/Wgj+5XDNuWZ1KJd11Y3qO36JF9C/wPOj9aq3NG11D+x5XqC f4CCEOKW51BUq7ec6g1qiXPNwme90ErPlW7SctS/e6wn9JAXsMF4fE+J5VzHMtbTpJjT 7FFRcACD2LVRt6nJKRidtf7TaAjmQ9riR/CVfeh0D7QcQfPpvJDf1pxYCkTrYX7dIg4m GXeHi2uXrzEET5q+EF2VsXQwxUNZknD/wy6RaitKu48fOVjliH01Dz6ZROa5rcwiVFGg 6/OQ== X-Gm-Message-State: AC+VfDzTaitIsGiPmlqYrfkumetNiJJ9ga5WN2meQ9kCLyLjXhrQN4xs mE3f3r2P0H+fQXdEWOG9QwNr6i6FD1/jFSY= X-Google-Smtp-Source: ACHHUZ6JmHF0O7xB25c4LZS3SBpbPu8gJ1LUrG2oig85p/mrkugigxAcAyjZYDiO5uEzwymT7yTtjwNZQO8JTzA= X-Received: from aliceryhl.c.googlers.com ([fda3:e722:ac3:cc00:31:98fb:c0a8:6c8]) (user=aliceryhl job=sendgmr) by 2002:a17:906:5a69:b0:974:5e62:2afc with SMTP id my41-20020a1709065a6900b009745e622afcmr502336ejc.13.1685698663640; Fri, 02 Jun 2023 02:37:43 -0700 (PDT) Date: Fri, 2 Jun 2023 09:37:41 +0000 In-Reply-To: Mime-Version: 1.0 References: X-Mailer: git-send-email 2.41.0.rc0.172.g3f132b7071-goog 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 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 List-ID: X-Mailing-List: rust-for-linux@vger.kernel.org Boqun Feng 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: RawWorkItem { >> + /// 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 { >> + /// The pointer type that this struct is wrapped in. This will typically be `Arc` or >> + /// `Pin>`. >> + type Pointer: WorkItemPointer; > > 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, > data: Data, > } > > impl Foo { > pub fn do_sth(&self) { > ... > } > } > > and we want to queue both Pin> and Arc as work items, but > the following doesn't work: > > // Pin> can be queued. > impl WorkItem for Foo { > type Pointer = Pin>; > fn run(ptr: Self::Pointer) { > ptr.do_sth(); > } > } > > // Arc can also be queued. > impl WorkItem for Foo { > type Pointer = Arc; > 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