From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f53.google.com (mail-wm1-f53.google.com [209.85.128.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CCB759462 for ; Tue, 30 May 2023 07:21:46 +0000 (UTC) Received: by mail-wm1-f53.google.com with SMTP id 5b1f17b1804b1-3f6d7abe934so27094665e9.2 for ; Tue, 30 May 2023 00:21:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=metaspace-dk.20221208.gappssmtp.com; s=20221208; t=1685431305; x=1688023305; h=mime-version:message-id:in-reply-to:date:subject:cc:to:from :user-agent:references:from:to:cc:subject:date:message-id:reply-to; bh=TLXAtxiSrzYBIqowst8fYYSmLsOfa+DzojwrE+Dj58Y=; b=lJ65t1rka6ry5GPPbgFan7QhZEsuQFbeK7qxuyt5ugdsnYtXxojvNRPyFnxr4+wjsy dzqq1O7PPwxmemEvb5ucIaysA2rwrdxDmvNOHIQ+r0KTYBIaUqGuPAzS6GkG8YpYH0q3 WR3H7/NKSPC3p0GymMBmR+CR3qjp1ieZy3vbaDOplHKbrMAqRq4Nzqn1efL93I1Mhywl Ga1mSYqsn++N/nKXwZ7LPz7T5STXUCRlnmqj6qVsx8KtPIo8dGNsPGBxvXhwwfk176iv RDZbcUnr+uZ/XA/tcwq39nbjoykBwULN27qtJHvSgxH3ghMl7C1gn0ilPoQjizz1xNVq 5QmA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685431305; x=1688023305; h=mime-version:message-id:in-reply-to:date:subject:cc:to:from :user-agent:references:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=TLXAtxiSrzYBIqowst8fYYSmLsOfa+DzojwrE+Dj58Y=; b=JO/w6hefsWYQyzSrbE6nful/NuTNUBKL7hH2oj9VYPN/sVMlLV+/UxTWoEHzY2hjxI X+IpoHcwk+PR/iUceeGTHhZV8bBTCXKZmcUuuCBjMUPZT0DWftW+kHbyIA0gAWuRYqqv ujkS9PL5yibLc//l14ozdLm8PMlP79TLq4uyug458FdOR3zutslPN5OXw6a5yzXGTgem fv+iA6GD/Q/TCsX907ViUWNlvFx8/V8h+0RI+Jo8vw5wMX09eqeRPrDnHniK90dnAMZs eXicQCLxNco5ib2ezNsHgZhljaCO16of4A14/SHLVkBnwqna3Ig8ygoZIZ44j2VeC/sp /lMw== X-Gm-Message-State: AC+VfDyKmVvQFmsPv9fERRX2I6RLEO0KjFSRIJC4PLdQyBJ7hatBqddM 3cDSHQTqBNUHSFWH1YpcZiOOzQ== X-Google-Smtp-Source: ACHHUZ4bpQk50zd1Sz0M23Ww94mlrU+hZmpxEGF/P7hHJ9PrpDsxwMDnpsbFzNuNRlXyTb0FAwRc3A== X-Received: by 2002:a1c:cc17:0:b0:3f5:772:f333 with SMTP id h23-20020a1ccc17000000b003f50772f333mr713950wmb.4.1685431304750; Tue, 30 May 2023 00:21:44 -0700 (PDT) Received: from localhost ([147.161.155.112]) by smtp.gmail.com with ESMTPSA id x11-20020a1c7c0b000000b003f50876905dsm16450769wmc.6.2023.05.30.00.21.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 May 2023 00:21:44 -0700 (PDT) References: <20230523110709.4077557-1-aliceryhl@google.com> User-agent: mu4e 1.10.3; emacs 28.2.50 From: Andreas Hindborg To: Alice Ryhl Cc: yakoyoku@gmail.com, alex.gaynor@gmail.com, benno.lossin@proton.me, bjorn3_gh@protonmail.com, boqun.feng@gmail.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 Subject: Re: [PATCH v1 6/7] rust: workqueue: add safe API to workqueue Date: Tue, 30 May 2023 09:19:52 +0200 In-reply-to: <20230523110709.4077557-1-aliceryhl@google.com> Message-ID: <875y8ab7y0.fsf@metaspace.dk> Precedence: bulk X-Mailing-List: patches@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain Alice Ryhl writes: > On 5/18/23 21:17, Martin Rodriguez Reboredo wrote: >> On 5/17/23 17:31, Alice Ryhl wrote: >>> +unsafe impl WorkItem for Arc >>> +where >>> + T: ArcWorkItem + HasWork + ?Sized, >>> +{ >>> + type EnqueueOutput = Result<(), Self>; >>> + >>> + unsafe fn __enqueue(self, queue_work_on: F) -> Self::EnqueueOutput >>> + where >>> + F: FnOnce(*mut bindings::work_struct) -> bool, >>> + { >>> + let ptr = Arc::into_raw(self); >>> + >>> + // Using `get_work_offset` here for object-safety. >>> + // >>> + // SAFETY: The pointer is valid since we just got it from `into_raw`. >>> + let off = unsafe { (&*ptr).get_work_offset() }; >>> + >>> + // SAFETY: The `HasWork` impl promises that this offset gives us a field of type >>> + // `Work` in the same allocation. >>> + let work_ptr = unsafe { (ptr as *const u8).add(off) as *const Work }; >>> + // SAFETY: The pointer is not dangling. >>> + let work_ptr = unsafe { Work::raw_get(work_ptr) }; >>> + >>> + match (queue_work_on)(work_ptr) { >> >> Match for boolean is not a good pattern in my eyes, if-else should be >> used instead. > > I think this is a question of style. For a comparison: > > match (queue_work_on)(work_ptr) { > true => Ok(()), > // SAFETY: The work queue has not taken ownership of the pointer. > false => Err(unsafe { Arc::from_raw(ptr) }), > } > > vs > > if (queue_work_on)(work_ptr) { > Ok(()) > } else { > // SAFETY: The work queue has not taken ownership of the pointer. > Err(unsafe { Arc::from_raw(ptr) }), > } > > I'm happy to change it if others disagree, but when the branches > evaluate to a short expression like they do here, I quite like the first > version. I prefer the first one, but both look OK to me. Is one more idiomatic than the other, or is it just a matter of personal preference? BR Andreas > >> Also aren't the parens around the closure unnecessary? > > Hmm, parenthesises are often required around closures, but it's possible > that it is only required for stuff like `self.closure(args)` to > disambiguate between a `closure` field (of pointer type) and a `closure` > method. I can check and remove them if they are not necessary.