Received-SPF: pass (google.com: domain of linux-kernel+bounces-131577-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240402-linked-list-v1-0-b1c59ba7ae3b@google.com>
 <20240402-linked-list-v1-2-b1c59ba7ae3b@google.com> <36306930-0390-4342-830d-03c11ec4a3a7@proton.me>
In-Reply-To: <36306930-0390-4342-830d-03c11ec4a3a7@proton.me>
From: Alice Ryhl <aliceryhl@google.com>
Date: Thu, 4 Apr 2024 16:14:14 +0200
Message-ID: <CAH5fLghHmuHxw1BL1F2-VF5KUXQLOwMqW6PfiAoLBXRE0Gyu=g@mail.gmail.com>
Subject: Re: [PATCH 2/9] rust: list: add tracking for ListArc
To: Benno Lossin <benno.lossin@proton.me>
Cc: Miguel Ojeda <ojeda@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, 
	Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, 
	Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, 
	=?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Andreas Hindborg <a.hindborg@samsung.com>, Marco Elver <elver@google.com>, 
	Kees Cook <keescook@chromium.org>, Coly Li <colyli@suse.de>, Paolo Abeni <pabeni@redhat.com>, 
	Pierre Gondois <pierre.gondois@arm.com>, Ingo Molnar <mingo@kernel.org>, 
	Jakub Kicinski <kuba@kernel.org>, Wei Yang <richard.weiyang@gmail.com>, 
	Matthew Wilcox <willy@infradead.org>, linux-kernel@vger.kernel.org, 
	rust-for-linux@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 3, 2024 at 5:52=E2=80=AFPM Benno Lossin <benno.lossin@proton.me=
> wrote:
>
> I think the commit one-line description sounds a bit strange, how about
> "rust: list: add ListArc tracking strategies"?
>
> On 02.04.24 14:16, Alice Ryhl wrote:
> > @@ -33,19 +34,64 @@ pub trait ListArcSafe<const ID: u64 =3D 0> {
> >      unsafe fn on_drop_list_arc(&self);
> >  }
> >
> > +/// Declares that this type is able to safely attempt to create `ListA=
rc`s at any time.
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that `try_new_list_arc` does not return `=
true` if a `ListArc` already
> > +/// exists.
> > +pub unsafe trait TryNewListArc<const ID: u64 =3D 0>: ListArcSafe<ID> {
> > +    /// Attempts to convert an `Arc<Self>` into an `ListArc<Self>`. Re=
turns `true` if the
> > +    /// conversion was successful.
> > +    fn try_new_list_arc(&self) -> bool;
> > +}
> > +
> >  /// Declares that this type supports [`ListArc`].
> >  ///
> > -/// When using this macro, it will only be possible to create a [`List=
Arc`] from a [`UniqueArc`].
> > +/// When using this macro, you may choose between the `untracked` stra=
tegy where it is not tracked
> > +/// whether a [`ListArc`] exists, and the `tracked_by` strategy where =
the tracking is deferred to a
> > +/// field of the struct. The `tracked_by` strategy can be combined wit=
h a field of type
> > +/// [`AtomicListArcTracker`] to track whether a [`ListArc`] exists.
> >  #[macro_export]
> >  macro_rules! impl_list_arc_safe {
> >      (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty { untra=
cked; } $($rest:tt)*) =3D> {
> > -        impl$(<$($generics)*>)? $crate::list::ListArcSafe<$num> for $t=
 {
> > +        impl$(<$($generics)*>)? ListArcSafe<$num> for $t {
>
> This change seems unintentional.

Will fix.

> >              unsafe fn on_create_list_arc_from_unique(&mut self) {}
> >              unsafe fn on_drop_list_arc(&self) {}
> >          }
> >          $crate::list::impl_list_arc_safe! { $($rest)* }
> >      };
> >
> > +    (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty {
> > +        tracked_by $field:ident : $fty:ty;
> > +    } $($rest:tt)*) =3D> {
> > +        impl$(<$($generics)*>)? ListArcSafe<$num> for $t {
>
> Here you also want to access `ListArcSafe` via
> `$crate::list::ListArcSafe`.

Will fix.

> > +            unsafe fn on_create_list_arc_from_unique(&mut self) {
> > +                let me =3D self as *mut Self;
> > +                let field: *mut $fty =3D unsafe { ::core::ptr::addr_of=
_mut!((*me).$field) };
>
> I think we should also have `SAFETY` comments in macros.
>
> Also why can't this be done using safe code?:
>
>      let field: &mut $fty =3D &mut self.$field;

Not sure why. Probably a historical accident. Will change it to be safe.

> > +                unsafe { <$fty as $crate::list::ListArcSafe<$num>>::on=
_create_list_arc_from_unique(
> > +                        &mut *field
> > +                ) };
>
> Formatting? rustfmt gives me this:
>
>                  unsafe {
>                      <$fty as $crate::list::ListArcSafe<$num>>::on_create=
_list_arc_from_unique(
>                          &mut *field
>                      )
>                  };
>
> (maybe the `;` should be inside the `unsafe` block in this case?)

I can make the change, but rustfmt does not affect macros.

Alice