On Thu, Nov 21, 2024 at 1:37 PM Lorenzo Stoakes
<lorenzo.stoakes@xxxxxxxxxx> wrote:
>
> I'm generally fine with this patch (other than rust specifics which I leave
> to the rust experts), however I'm a little concerned about us taking
> reference counts when we don't need to so this is something I'd like to see
> addressed for v9 or, at least to be confident we're not doing this in the
> binder code unnecessarily.
>
> Thanks!
The refcount increment is *not* unnecessary in Binder. For the C
equivalent, see the implementation of `binder_alloc_init`. It's used
because Binder will access the mm of the recipient from the sender's
scope, so it must hold on to an mm_struct reference.
> > > > +/// [`mmget_not_zero`]: Mm::mmget_not_zero
> > > > +#[repr(transparent)]
> > > > +pub struct Mm {
> > > > + mm: Opaque<bindings::mm_struct>,
> > > > +}
> > >
> > > Does this tie this type to the C struct mm_struct type?
> > >
> > > Does 'Opaque' mean it is a pointer to a type which is 'opaque' in the sense
> > > that rust can't see into its internals?
> >
> > This declaration defines a Rust type called Mm which has the same
> > size, alignment, and contents as `struct mm_struct`. The purpose of
> > Opaque is to tell Rust that it can't assume anything about the
> > contents at all; we do that to leave it up to C.
> >
> > For example, normally if you have an immutable reference &i32, then
> > Rust is going to assume that the contents behind the reference are in
> > fact immutable. Opaque turns that off, meaning that an `&Opaque<i32>`
> > is allowed to reference an integer as it gets modified. It makes all
> > access to the contents unsafe, though.
> >
> > Note that Opaque is *not* a pointer type. We're going to be dealing
> > with types like &Mm or ARef<Mm> where &_ and ARef<_> are two different
> > kinds of pointers.
>
> OK so when you reference Mm.mm that is in effect referencing the struct
> mm_struct object rather than a pointer to a struct mm_struct? and
Yes.
> > > > +// SAFETY: By the type invariants, this type is always refcounted.
> > > > +unsafe impl AlwaysRefCounted for Mm {
> > > > + #[inline]
> > > > + fn inc_ref(&self) {
> > > > + // SAFETY: The pointer is valid since self is a reference.
> > > > + unsafe { bindings::mmgrab(self.as_raw()) };
> > > > + }
> > > > +
> > > > + #[inline]
> > > > + unsafe fn dec_ref(obj: NonNull<Self>) {
> > > > + // SAFETY: The caller is giving up their refcount.
> > > > + unsafe { bindings::mmdrop(obj.cast().as_ptr()) };
> > > > + }
> > > > +}
> > >
> > > Under what circumstances would these be taken? Same question for MmWthUser.
> >
> > This makes `Mm` compatible with the pointer type called ARef<_>. This
> > pointer type is used to represent ownership of a refcount. So whenever
> > a variable of type ARef<_> goes out of scope, the refcount is
> > decremented, and whenever an ARef<_> is cloned, the refcount is
> > incremented.
> >
> > The way this works is that ARef<_> is implemented to use the
> > AlwaysRefCounted trait to understand how to manipulate the count. Only
> > types that implement the trait with an impl block like above can be
> > used with ARef<_>.
>
> OK so when you first instantiate an ARef<_> you don't increment the
> reference count, only if it is cloned from there on in?
Well it depends on which ARef constructor you are using. But the uses
of ARef::from_raw do not increment the count.
> > > > +// These methods are safe to call even if `mm_users` is zero.
> > > > +impl Mm {
> > > > + /// Call `mmgrab` on `current.mm`.
> > > > + #[inline]
> > > > + pub fn mmgrab_current() -> Option<ARef<Mm>> {
> > > > + // SAFETY: It's safe to get the `mm` field from current.
> > > > + let mm = unsafe {
> > > > + let current = bindings::get_current();
> > > > + (*current).mm
> > > > + };
> > >
> > > I don't see an equivalent means of obtaining mm from current for
> > > MmWithUser, is that intentional, would there be another means of obtaining
> > > an mm? (perhaps via vma->vm_mm I guess?)
> > >
> > > An aside -->
> > >
> > > If we're grabbing from current, and this is non-NULL (i.e. not a kernel
> > > thread), this is kinda MmWithUser to _start out_ with, but I guess if
> > > you're grabbing the current one you might not expect it.
> > >
> > > I guess one thing I want to point out (maybe here is wrong place) is that
> > > the usual way of interacting with current->mm is that we do _not_ increment
> > > mm->mm_count, mm->mm_users or any refernce count, as while we are in the
> > > kernel portion of the task, we are guaranteed the mm and the backing
> > > virtual address space will stick around.
> > >
> > > With reference to MmWithUser, in fact, if you look up users of
> > > mmget()/mmput() it is pretty rare to do that.
> > >
> > > So ideally we'd avoid doing this if we could (though having the semantics
> > > of grabbing a reference if we were to copy the object somehow again or hold
> > > its state or something would be nice).
> > >
> > > I guess this might actually be tricky in rust, because we'd probably need
> > > to somehow express the current task's lifetime and tie this to that
> > > and... yeah.
> > >
> > > <-- aside
> >
> > Ah, yeah, I guess this API is incomplete. We could have an API that
> > lets you obtain an &MmWithUser instead. Then, if the user wants to
> > increment the refcount, they can manually convert that into an
> > ARef<Mm> or ARef<MmWithUser>.
> >
> > It's true that it's slightly tricky to express in Rust, but it's
> > possible. We already have a way to get a &Task pointing at current.
>
> Yeah, but I think we should do this incrementally as I want this initial
> series to merge first, after which we can tweak things.
Rest assured that the API can be written to not automatically
increment the refcount when accessing current. That's just binder's
case.
> > > > + unsafe { &*ptr.cast() }
> > > > + }
> > > > +
> > > > + /// Calls `mmget_not_zero` and returns a handle if it succeeds.
> > > > + #[inline]
> > > > + pub fn mmget_not_zero(&self) -> Option<ARef<MmWithUser>> {
> > >
> > > I actually kinda love that this takes an mm and guarantees that you get an
> > > MmWithUser out of it which is implied by the fact this succeeds.
> > >
> > > However as to the point above, I'm concerned that this might be seen as
> > > 'the way' to access an mm, i.e. mm.mmgrab_current().mmget_not_zero() or
> > > something.
> > >
> > > Whereas, the usual way of referencing current->mm is to not increment any
> > > reference counts at all (assuming what you are doing resides in the same
> > > lifetime as the task).
> > >
> > > Obviously if you step outside of that lifetime, then you _do_ have to pin
> > > the mm (nearly always you want to grab rather than get though in that
> > > circumstance).
> >
> > I can add a way to obtain an &MmWithUser from current without
> > incrementing the refcount.
>
> Yeah, I feel like we definitely need this.
>
> However we'd need to _not_ drop the refcount when it goes out of scope too
> in this case.
>
> I'm not sure how you'd express that, however.
The way you express that is by giving the user a &Mm or &MmWithUser
instead of giving the user an ARef<Mm> or ARef<MmWithUser>. Using a
normal reference implies that you don't have ownership over the
refcount, and the reference has no destructor when it goes out of
scope. The only slightly tricky piece is tying the lifetime of that
reference to the scope of the current task, but this is a problem with
a known solution.
For more on this, see the discussion on the various versions of
Christian's PidNamespace series:
https://lore.kernel.org/rust-for-linux/20241002-brauner-rust-pid_namespace-v5-1-a90e70d44fde@xxxxxxxxxx/
Alice
