"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`. > 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? >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. Best regards, Andreas Hindborg
