Re: [PATCH v20 4/8] rust: page: convert to `Ownable`'

"Alice Ryhl" [off-list ref] writes:

On Tue, Sep 8, 2026 at 11:00 AM Andreas Hindborg [off-list ref] wrote:

quoted

Alice Ryhl [off-list ref] writes:

quoted

On Mon, Sep 7, 2026 at 3:58 PM Andreas Hindborg [off-list ref] wrote:

quoted

"Alice Ryhl" [off-list ref] writes:

quoted

On Mon, Sep 7, 2026 at 2:38 PM Andreas Hindborg [off-list ref] wrote:

quoted

Alice Ryhl [off-list ref] writes:

quoted

On Sun, Sep 6, 2026 at 3:02 PM Gary Guo [off-list ref] wrote:

quoted

On Tue Aug 25, 2026 at 2:20 PM BST, Alice Ryhl wrote:

quoted

On Mon, Aug 24, 2026 at 01:17:56PM +0200, Andreas Hindborg wrote:

quoted

+        // SAFETY: We just successfully allocated a page, so we now have ownership of the newly
+        // allocated page. We transfer that ownership to the new `Owned<Page>` object.
+        // Since `Page` is transparent, we can cast the pointer directly.
+        Ok(unsafe { Owned::from_raw(page.cast()) })

This doesn't satisfy the safety requirements of Owned::from_raw()
because the page may be used with vm_insert_page(), which increments its
refcount and causes it to be shared the vma system, and this occurs
before Page::release() is called.

I suppose the existing vm_insert_page() abstraction we have is already
problematic, because it uses `&Page`?

Maybe we want to change the API to use `ARef<Page>` so it already has to be
shared? Conceptually it takes a reference count from a `&Page`, which isn't
possible because `Page` is not `AlwaysRefCounted`, so it needs a `&ARef<Page>`
to be able to do that op.

Honestly, the problem is the safety requirements of Owned::from_raw().
Pages have a "special" main reference, and free_page() does more than
put_page(). It even does something when the refcount does not hit
zero.

The correct behavior for Page is to allow the user to hold one
Owned<Page> whose drop calls free_page(), *plus* any number of
ARef<Page> references that invoke put_page() on drop. This way, the
owned page controls the special drop codepath.

This does not mesh well with the model of `Owned` behaving like
`UniqueArc` to `ARef` behaving like an `Arc`.

It's a different case, I agree.

quoted

Please help me understand; with page having a main ref and an auxiliary
refcount, if we model that with a single `Owned<Page>` and a number of
`ARef<Page>`, what would happen in the case where the main ref (`Owned`)
is dropped first? Is this legal?

Yes, it's legal.

quoted

My intuition here would be to follow Garry's suggestion and have
`vm_insert_page` take an `ARef<Page>`. Can you elaborate why this is not
an option?

That would be the wrong ownership semantics. The C side increments the
refcount rather than take ownership of a passed-in refcount, so the
correct argument type is &Page.

Or `&ARef<Page>`? Because with `Page` being `Ownable` it would not be
OK to increment the refcount from just a `&Page`.

That's just a limitation in your traits. There is no problem with
creating a refcount from just a &Page.

Yes, I agree. The limitation is by design. My point is, `vm_insert_page`
can work fine with the `&ARef<Page>` type. So if that would be OK with
you, I would patch that as part of this patch.

IMO, Page should implement AlwaysRefcounted and this method should
take &Page. Anything else just adds limitations that are not needed.
But I'm ok not implementing Ownable for it.

Ok, I think you have me convinced. Mainly because `Page` as it is has no
benefit of exclusive access.

quoted

quoted

quoted

quoted

Now, for the use-case in Binder I believe we could switch to
put_page(), as we don't need the extra stuff from free_page(), in
which case we only want ARef<Page> and do not require Owned support.
But as long as we are calling free_page(), it needs to not be
clonable.

You are calling `__free_page` directly in binder? I could not find this
with grep except in the C binder. Is this code you are calling into from
Rust binder?

Via the Drop of Page, not directly of course.

quoted

From `__free_pages()` kernel-doc (`mm/page_alloc.c`):

quoted

If the last reference to this page is speculative, it will be
released by put_page() which only frees the first page of a
non-compound allocation. [...] **If you want to use the page's
reference count to decide when to free the allocation, you should
allocate a compound page, and use put_page() instead of
__free_pages().**

I guess this is what you are referring to? We could change `Page` to
this style as well and all would be fine I think?

At any rate, today I learned about Pfn walkers, compaction,
memory-failure, etc. that take transient refcounts
pages. With that in mind we need to reformulate the invariant of
`Ownable` to exclude optimistic transient references.

Yes we can switch Rust Binder to use put_page() and have Page not
implement Ownable at all. That would be okay, because Binder's pages
are not higher order.

As far as I can tell, we can also switch `Owned<Page>` to allocate the
page with `__GFP_COMP` and use `put_page` in drop.

I'm not entirely sure if __GFP_COMP is needed, since the page is order
zero to begin with, so I don't think it makes a difference.

With alignment on `Page: AlwaysRefCounted`, I think this becomes
orthogonal. When we want to support allocations of more than one page,
we should probably make them compound pages (folios) and manage them
with the refcount (put_page), rather than non-compound high-order pages.
But this can wait.

quoted

quoted

But this case still does highlight that
Ownable<_> currently does not address the use-case where the
Ownable<T> is a special reference there can only be one of, rather
than a completely unique reference.

This is not the use case `Ownable` aims to solve.

Maybe not, but then it doesn't apply to Page.

I agree.

quoted

quoted

For instance, the GPUVM abstraction would be another case where
Ownable<_> could be used if we have this other model where Ownable<_>
and ARef<_> are legal at the same time. Currently GPUVM uses a special
UniqueRefGpuVm type to solve this problem.

I am wondering if we can make case I intend to solve (Ownable is a
unique reference) coexist with the "at most one of this ref can exist".
The "at most one of this kind" sounds similar to the `ListArc` use case
to me, which is a different problem.

I think they can co-exist. If AlwaysRefcounted is implemented, it is
the ListArc use-case. If Refcounted but not AlwaysRefcounted is
implemented, it is UniqueArc use-case.

I see.

So for v21 I will implement RefCounted + AlwaysRefCounted for Page and
drop the Ownable implementation, have alloc_page() return ARef<Page>,
keep vm_insert_page() taking &Page, and switch Rust Binder to hold
ARef<Page>.

I will also include the SafePage/ExclusivePage series [1] in v21 as a user
of `Ownable` with a few modifications. It will no longer be possible for
this type to deref to `&Page`, due to `Page: AlwaysRefCounted` and
`vm_insert_page` taking refcounts through `&Page`.

Best regards,
Andreas Hindborg

[1] https://lore.kernel.org/rust-for-linux/20260605-page-additions-v2-0-03f04c8fdbbf@kernel.org/ (local)