From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-40134.protonmail.ch (mail-40134.protonmail.ch [185.70.40.134]) (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 3E89A3C30 for ; Fri, 26 Jul 2024 07:41:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.70.40.134 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721979668; cv=none; b=MA5gpnq6txQRLQ4dWzOMyAoMxIVt51Wsi+jdkFzUGhXFOYFEzidS/GUYLhJ1Axj44kjUDflasZxmc/AeFGcS/20JXRQuBOUf9LW5tB0AXQ089XorfWkHtPCv4OQB+XuiJ8Cu9FfP3jYWPFJD5adsEuIjwFSwO8mmGYRkBP4zO14= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721979668; c=relaxed/simple; bh=I70QW6hpY+jLdfy5HkEgw5Gl1NKikX5wbk22K6+C/eQ=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=UYnDfpWB2j/tIPPVMPxqPgn0cc7qyx9SDg1sHZZJgqthIpIDWTbHOj2vjIrhzz7mivAcDJyjme+MaWtFB7+8LwPKHUOQrbEQserz/HEVyxecwfMm/meYCKoQa0zpELFW5wGIJlj8fx6Cf9KYtUCF5vCVrfUd0oFH3FduFCEY4qk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me; spf=pass smtp.mailfrom=proton.me; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b=cBR5Dq8P; arc=none smtp.client-ip=185.70.40.134 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=proton.me Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=proton.me Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=proton.me header.i=@proton.me header.b="cBR5Dq8P" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1721979664; x=1722238864; bh=nuNNEuDtKjzAbgP3yLhd9hSlLZ1VjY9HwALtq8R4bIs=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=cBR5Dq8P7o5mkS9xOC4P/s/vArJK0fYrmVvGA30eEvSd20DfFODGSr+0lglaWhpaI ZhLfR+R0o+LeNhP2k1hkDEk7e5YQeVfdsaSzXbheCZNUlfAXULhRBAASCxyFAyrHGh 1sfH9w0FOnX4Iz6GjhtSRyZX2VSXuHr9dR382dSj3tXUqg5xKrLng5KbL7gKmLxBdP VGWLxEsa735t/EtQQLRCuBsG2n7qP2gTe3ah2dCQ47jBZXTOhgSkzzw3ak4ueq7rK4 YekLlrWbcYZOT9nrL+awjnEM8zC0Cmzoyom2Cj6QNi2gIryz+d/hZZMivkRtg6ul9/ if0j/0K+dnQjQ== Date: Fri, 26 Jul 2024 07:40:59 +0000 To: Lyude Paul , rust-for-linux@vger.kernel.org From: Benno Lossin Cc: Danilo Krummrich , airlied@redhat.com, Ingo Molnar , Will Deacon , Waiman Long , Peter Zijlstra , Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Alice Ryhl , Martin Rodriguez Reboredo , Valentin Obst , Trevor Gross , Ben Gooding , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/3] rust: sync: Introduce LockContainer trait Message-ID: <59515c1e-d1f4-47c3-a201-d2b0824f948b@proton.me> In-Reply-To: <20240725222822.1784931-3-lyude@redhat.com> References: <20240725222822.1784931-1-lyude@redhat.com> <20240725222822.1784931-3-lyude@redhat.com> Feedback-ID: 71780778:user:proton X-Pm-Message-ID: 8520140208c81d9550b798ed5e0975a1d64dec95 Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 26.07.24 00:27, Lyude Paul wrote: > We want to be able to use spinlocks in no-interrupt contexts, but our > current `Lock` infrastructure doesn't allow for the ability to pass > arguments when acquiring a lock - meaning that there would be no way for = us > to verify interrupts are disabled before granting a lock since we have > nowhere to pass an `IrqGuard`. >=20 > It doesn't particularly made sense for us to add the ability to pass such > an argument either: this would technically work, but then we would have t= o > pass empty units as arguments on all of the many locks that are not grabb= ed > under interrupts. As a result, we go with a slightly nicer solution: I think there is a solution that would allow us to have both[1]: 1. Add a new associated type to `Backend` called `Context`. 2. Add a new parameter to `Backend::lock`: `ctx: Self::Context`. 3. Add a new function to `Lock`: `lock_with(&self, ctx: B::Context)` that delegates to `B::lock`. 4. Reimplement `Lock::lock` in terms of `Lock::lock_with`, by constraining the function to only be callable if `B::Context: Default` holds (and then using `Default::default()` as the value). This way people can still use `lock()` as usual, but we can also have `lock_with(irq)` for locks that require it. [1]: I think I saw this kind of a pattern first from Wedson in the context of passing default allocation flags. > introducing a trait for types which can contain a lock of a specific type= : > LockContainer. This means we can still use locks implemented on top of > other lock types in types such as `LockedBy` - as we convert `LockedBy` t= o > begin using `LockContainer` internally and implement the trait for all > existing lock types. >=20 > Signed-off-by: Lyude Paul > --- > rust/kernel/sync.rs | 1 + > rust/kernel/sync/lock.rs | 20 ++++++++++++++++++++ > rust/kernel/sync/locked_by.rs | 11 +++++++++-- > 3 files changed, 30 insertions(+), 2 deletions(-) >=20 > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > index 0ab20975a3b5d..14a79ebbb42d5 100644 > --- a/rust/kernel/sync.rs > +++ b/rust/kernel/sync.rs > @@ -16,6 +16,7 @@ > pub use condvar::{new_condvar, CondVar, CondVarTimeoutResult}; > pub use lock::mutex::{new_mutex, Mutex}; > pub use lock::spinlock::{new_spinlock, SpinLock}; > +pub use lock::LockContainer; > pub use locked_by::LockedBy; >=20 > /// Represents a lockdep class. It's a wrapper around C's `lock_class_ke= y`. > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs > index f6c34ca4d819f..bbd0a7465cae3 100644 > --- a/rust/kernel/sync/lock.rs > +++ b/rust/kernel/sync/lock.rs > @@ -195,3 +195,23 @@ pub(crate) unsafe fn new(lock: &'a Lock, state= : B::GuardState) -> Self { > } > } > } > + > +/// A trait implemented by any type which contains a [`Lock`] with a spe= cific [`Backend`]. > +pub trait LockContainer { > + /// Returns an immutable reference to the lock > + /// > + /// # Safety > + /// > + /// Since this returns a reference to the contained [`Lock`] without= going through the > + /// [`LockContainer`] implementor, it cannot be guaranteed that it i= s safe to acquire > + /// this lock. Thus the caller must promise not to attempt to use th= e returned immutable > + /// reference to attempt to grab the underlying lock without ensurin= g whatever guarantees the > + /// [`LockContainer`] implementor's interface enforces. This safety requirement is rather unclear to me, there isn't really a good place to put the `LockContainer` requirements when implementing this trait. I also don't understand the use-case where a lock can only be acquired in certain circumstances, do you have an example? --- Cheers, Benno > + unsafe fn get_lock_ref(&self) -> &Lock; > +} > + > +impl LockContainer for Lock { > + unsafe fn get_lock_ref(&self) -> &Lock { > + &self > + } > +} > diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.r= s > index babc731bd5f62..d16d89fe74e0b 100644 > --- a/rust/kernel/sync/locked_by.rs > +++ b/rust/kernel/sync/locked_by.rs > @@ -95,13 +95,20 @@ impl LockedBy { > /// data becomes inaccessible; if another instance of the owner is a= llocated *on the same > /// memory location*, the data becomes accessible again: none of thi= s affects memory safety > /// because in any case at most one thread (or CPU) can access the p= rotected data at a time. > - pub fn new(owner: &Lock, data: T) -> Self { > + pub fn new(owner: &L, data: T) -> Self > + where > + B: Backend, > + L: super::LockContainer, > + { > build_assert!( > size_of::>() > 0, > "The lock type cannot be a ZST because it may be impossible = to distinguish instances" > ); > Self { > - owner: owner.data.get(), > + // SAFETY: We never directly acquire the lock through this r= eference, we simply use it > + // to ensure that a `Guard` the user provides us to access t= his container's contents > + // belongs to the same lock that owns this data > + owner: unsafe { owner.get_lock_ref() }.data.get(), > data: UnsafeCell::new(data), > } > } > -- > 2.45.2 >=20