mbox series

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

Message ID 20241119112408.779243-1-abdiel.janulgue@gmail.com (mailing list archive)
Headers show
Series rust: page: Add support for existing struct page mappings | expand

Message

Abdiel Janulgue Nov. 19, 2024, 11:24 a.m. UTC
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).
- Make Page::page_slice_to_page handle virt_to_page cases as well
  (Danilo Krummrich).
- Link to v2: https://lore.kernel.org/lkml/20241022224832.1505432-1-abdiel.janulgue@gmail.com/

Changes since v2:
- Use Owned and Ownable types for constructing Page as suggested in
  instad of using ptr::read().
- Link to v1: https://lore.kernel.org/rust-for-linux/20241007202752.3096472-1-abdiel.janulgue@gmail.com/

Abdiel Janulgue (2):
  rust: page: use the page's reference count to decide when to free the
    allocation
  rust: page: Extend support to existing struct page mappings

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/page.c             |  20 +++++
 rust/kernel/page.rs             | 135 ++++++++++++++++++++++++++++----
 3 files changed, 142 insertions(+), 14 deletions(-)


base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb

Comments

Matthew Wilcox Nov. 20, 2024, 4:57 a.m. UTC | #1
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.

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?
Alice Ryhl Nov. 20, 2024, 9:10 a.m. UTC | #2
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.
>
> 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?

Alice
Boqun Feng Nov. 20, 2024, 4:20 p.m. UTC | #3
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?)

> > 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?
> 

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.

Regards,
Boqun

> Alice
Matthew Wilcox Nov. 20, 2024, 5:02 p.m. UTC | #4
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.

> > > 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
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 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.
Boqun Feng Nov. 20, 2024, 5:25 p.m. UTC | #5
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?

Regards,
Boqun
Abdiel Janulgue Nov. 20, 2024, 10:56 p.m. UTC | #6
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? :)

Regards,
Abdiel
Boqun Feng Nov. 21, 2024, 12:24 a.m. UTC | #7
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.

Regards,
Boqun

> Regards,
> Abdiel
Alice Ryhl Nov. 21, 2024, 9:19 a.m. UTC | #8
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
Abdiel Janulgue Nov. 21, 2024, 9:30 a.m. UTC | #9
Hi Boqun, Matthew:

On 21/11/2024 02:24, Boqun Feng wrote:
>>> 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.
> 

I'm not sure. Is there a way to get the struct folio from a vmalloc'd 
address, e.g vmalloc_to_folio()?

Regards,
Abdiel