Re: [PATCH v3 0/2] rust: page: Add support for existing struct page mappings

[Alice Ryhl <aliceryhl@google.com>]

On Thu, Nov 21, 2024 at 1:24 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Thu, Nov 21, 2024 at 12:56:38AM +0200, Abdiel Janulgue wrote:
> > On 20/11/2024 19:25, Boqun Feng wrote:
> > > On Wed, Nov 20, 2024 at 05:02:14PM +0000, Matthew Wilcox wrote:
> > > > On Wed, Nov 20, 2024 at 08:20:16AM -0800, Boqun Feng wrote:
> > > > > On Wed, Nov 20, 2024 at 10:10:44AM +0100, Alice Ryhl wrote:
> > > > > > On Wed, Nov 20, 2024 at 5:57 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > >
> > > > > > > On Tue, Nov 19, 2024 at 01:24:01PM +0200, Abdiel Janulgue wrote:
> > > > > > > > This series aims to add support for pages that are not constructed by an
> > > > > > > > instance of the rust Page abstraction, for example those returned by
> > > > > > > > vmalloc_to_page() or virt_to_page().
> > > > > > > >
> > > > > > > > Changes sinve v3:
> > > > > > > > - Use the struct page's reference count to decide when to free the
> > > > > > > >    allocation (Alice Ryhl, Boqun Feng).
> > > > > > >
> > > > > > > Bleh, this is going to be "exciting".  We're in the middle of a multi-year
> > > > > > > project to remove refcounts from struct page.  The lifetime of a page
> > > > > > > will be controlled by the memdesc that it belongs to.  Some of those
> > > > > > > memdescs will have refcounts, but others will not.
> > > > > > >
> > > > >
> > > > > One question: will the page that doesn't have refcounts has an exclusive
> > > > > owner? I.e. there is one owner that's responsible to free the page and
> > > > > make sure other references to the page get properly invalidated (maybe
> > > > > via RCU?)
> > > >
> > > > It's up to the owner of the page how they want to manage freeing it.
> > > > They can use a refcount (folios will still have a refcount, for example),
> > > > or they can know when there are no more users of the page (eg slab knows
> > > > when all objects in a slab are freed).  RCU is a possibility, but would
> > > > be quite unusual I would think.  The model I'm looking for here is that
> > > > 'page' is too low-level an object to have its own lifecycle; it's always
> > > > defined by a higher level object.
> > > >
> > >
> > > Ok, that makes sense. That's actually aligned with the direction we are
> > > heading in this patch: make `struct Page` itself independent on how the
> > > lifetime is maintained. Conceptually, say we can define folio in pure
> > > Rust, it could be:
> > >
> > >      struct Folio {
> > >          head: Page, /* or a union of page */
> > >     ...
> > >      }
> > >
> > > and we can `impl AlwaysRefcounted for Folio`, which implies there is a
> > > refcount inside. And we can also have a `Foo` being:
> > >
> > >      struct Foo {
> > >          inner: Page,
> > >      }
> > >
> > > which doesn't implement `AlwaysRefcounted`, and that suggests a
> > > different way the page lifetime will be maintained.
> > >
> > > > > > > We don't have a fully formed destination yet, so I can't give you a
> > > > > > > definite answer to a lot of questions.  Obviously I don't want to hold
> > > > > > > up the Rust project in any way, but I need to know that what we're trying
> > > > > > > to do will be expressible in Rust.
> > > > > > >
> > > > > > > Can we avoid referring to a page's refcount?
> > > > > >
> > > > > > I don't think this patch needs the refcount at all, and the previous
> > > > > > version did not expose it. This came out of the advice to use put_page
> > > > > > over free_page. Does this mean that we should switch to put_page but
> > > > > > not use get_page?
> > > >
> > > > Did I advise using put_page() over free_page()?  I hope I didn't say
> > >
> > > We have some off-list discussion about free_page() doesn't always free
> > > the page if you could remember.
> > >
> > > > that.  I don't see a reason why binder needs to refcount its pages (nor
> > > > use a mapcount on them), but I don't fully understand binder so maybe
> > > > it does need a refcount.
> > >
> > > I don't think binder needs it either, but I think Abdiel here has a
> > > different usage than binder.
> > >
> > > >
> > > > > I think the point is finding the exact lifetime model for pages, if it's
> > > > > not a simple refcounting, then what it is? Besides, we can still
> > > > > represent refcounting pages with `struct Page` and other pages with a
> > > > > different type name. So as far as I can see, this patch is OK for now.
> > > >
> > > > I don't want Page to have a refcount.  If you need something with a
> > > > refcount, it needs to be called something else.
> > >
> > > So if I understand correctly, what Abdiel needs here is a way to convert
> > > a virtual address to the corresponding page, would it make sense to just
> > > use folio in this case? Abdiel, what's the operation you are going to
> > > call on the page you get?
> >
> > Yes that's basically it. The goal here is represent those existing struct
> > page within this rust Page abstraction but at the same time to avoid taking
> > over its ownership.
> >
> > Boqun, Alice, should we reconsider Ownable and Owned trait again? :)
> >
>
> Could you use folio in your case? If so, we can provide a simple binding
> for folio which should be `AlwaysRefcounted`, and re-investigate how
> page should be wrapped.

Well, regardless of that, I do think it sounds like Owned / Ownable is
the right way forward for Page.

Alice