Re: [PATCH 0/6] rust: page: Support borrowing `struct page` and physaddr conversion

[David Hildenbrand <david@redhat.com>]

On 03.02.25 22:05, Zi Yan wrote:
> On 3 Feb 2025, at 9:32, Asahi Lina wrote:
> 
>> On 2/3/25 6:58 PM, Simona Vetter wrote:
>>> On Sun, Feb 02, 2025 at 10:05:42PM +0900, Asahi Lina wrote:
>>>> This series refactors the existing Page wrapper to support borrowing
>>>> `struct page` objects without ownership on the Rust side, and converting
>>>> page references to/from physical memory addresses.
>>>>
>>>> The series overlaps with the earlier submission in [1] and follows a
>>>> different approach, based on the discussion that happened there.
>>>>
>>>> The primary use case for this is implementing IOMMU-style page table
>>>> management in Rust. This allows drivers for IOMMUs and MMU-containing
>>>> SoC devices to be written in Rust (such as embedded GPUs). The intended
>>>> logic is similar to how ARM SMMU page tables are managed in the
>>>> drivers/iommu tree.
>>>>
>>>> First, introduce a concept of Owned<T> and an Ownable trait. These are
>>>> similar to ARef<T> and AlwaysRefCounted, but are used for types which
>>>> are not ref counted but rather have a single intended owner.
>>>>
>>>> Then, refactor the existing Page support to use the new mechanism. Pages
>>>> returned from the page allocator are not intended to be ref counted by
>>>> consumers (see previous discussion in [1]), so this keeps Rust's view of
>>>> page ownership as a simple "owned or not". Of course, this is still
>>>> composable as Arc<Owned<Page>> if Rust code needs to reference count its
>>>> own Page allocations for whatever reason.
>>>
>>> I think there's a bit a potential mess here because the conversion to
>>> folios isn't far enough yet that we can entirely ignore page refcounts and
>>> just use folio refcounts. But I guess we can deal with that oddity if we
>>> hit it (maybe folio conversion moves fast enough), since this only really
>>> starts to become relevant for hmm/svm gpu stuff.
>>>
>>> iow I think anticipating the future where struct page really doesn't have
>>> a refcount is the right move. Aside from that it's really not a refcount
>>> that works in the rust ARef sense, since struct page cannot disappear for
>>> system memory, and for dev_pagemap memory it's an entirely different
>>> reference you need (and then there's a few more special cases).
>>
>> Right, as far as this abstraction is concerned, all that needs to hold
>> is that:
>>
>> - alloc_pages() and __free_pages() work as intended, however that may
>> be, to reserve and return one page (for now, though I think extending
>> the Rust abstraction to handle higher-order folios is pretty easy, but
>> that can happen later).
>> - Whatever borrows pages knows what it's doing. In this case there's
>> only support for borrowing pages by physaddr, and it's only going to be
>> used in a driver for a platform without memory hot remove (so far) and
>> only for pages which have known usage (in principle) and are either
>> explicitly allocated or known pinned or reserved, so it's not a problem
>> right now. Future abstractions that return borrowed pages can do their
>> own locking/bookkeeping/whatever is necessary to keep it safe.
>>
>> I would like to hear how memory hot-remove is supposed to work though,
>> to see if we should be doing something to make the abstraction safer
>> (though it's still unsafe and always will be). Is there a chance a
>> `struct page` could vanish out from under us under some conditions?
> 
> Add DavidH and OscarS for memory hot-remove questions.
> 
> IIUC, struct page could be freed if a chunk of memory is hot-removed.

Right, but only after there are no users anymore (IOW, memory was freed 
back to the buddy). PFN walkers might still stumble over them, but I 
would not expect (or recommend) rust to do that.

> 
> Another case struct page can be freed is when hugetlb vmemmap optimization
> is used. Muchun (cc'd) is the maintainer of hugetlbfs.

Here, the "struct page" remains valid though; it can still be accessed, 
although we disallow writes (which would be wrong).

If you only allocate a page and free it later, there is no need to worry 
about either on the rust side.

-- 
Cheers,

David / dhildenb