"Alice Ryhl" <[email protected]> writes: > On Tue, Sep 8, 2026 at 11:00 AM Andreas Hindborg <[email protected]> > wrote: >> >> Alice Ryhl <[email protected]> writes: >> >> > On Mon, Sep 7, 2026 at 3:58 PM Andreas Hindborg <[email protected]> >> > wrote: >> >> >> >> "Alice Ryhl" <[email protected]> writes: >> >> >> >> > On Mon, Sep 7, 2026 at 2:38 PM Andreas Hindborg <[email protected]> >> >> > wrote: >> >> >> >> >> >> Alice Ryhl <[email protected]> writes: >> >> >> >> >> >> > On Sun, Sep 6, 2026 at 3:02 PM Gary Guo <[email protected]> wrote: >> >> >> >> >> >> >> >> On Tue Aug 25, 2026 at 2:20 PM BST, Alice Ryhl wrote: >> >> >> >> > On Mon, Aug 24, 2026 at 01:17:56PM +0200, Andreas Hindborg wrote: >> >> >> >> >> + // 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. >> >> > >> >> >> 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. >> >> > >> >> >> 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. >> >> > 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. >> > >> >> From `__free_pages()` kernel-doc (`mm/page_alloc.c`): >> >> >> >> > 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. >> > 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. >> > 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/[email protected]/
