From: Abdiel Janulgue <abdiel.janulgue@gmail.com>
To: Alice Ryhl <aliceryhl@google.com>, Boqun Feng <boqun.feng@gmail.com>
Cc: rust-for-linux@vger.kernel.org, dakr@redhat.com,
linux-kernel@vger.kernel.org, airlied@redhat.com,
miguel.ojeda.sandonis@gmail.com
Subject: Re: [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait
Date: Fri, 1 Nov 2024 15:38:16 +0200 [thread overview]
Message-ID: <fe25c0a4-7e1b-45f3-b413-c52d033c7906@gmail.com> (raw)
In-Reply-To: <CAH5fLgjLouU9ZRabJtP9qK6RWNLHZvW6dtUqbCkzFqZO+9skTQ@mail.gmail.com>
Hi Alice, Boqun:
On 24/10/2024 10:33, Alice Ryhl wrote:
>>>>>>>
>>>>>>> Please rename this function to from_raw to match the name used by
>>>>>>> other similar functions.
>>>>>>>
>>>>>>> Also, I don't love this wording. We don't really want to guarantee
>>>>>>> that it is unique. For example, pages have one primary owner, but
>>>>>>> there can be others who also have refcounts to the page, so it's not
>>>>>>> really unique. I think you just want to say that `ptr` must point at a
>>>>>
>>>>> But then when `Owned<Page>` dropped, it will call __free_pages() which
>>>>> invalidate any other existing users. Do you assume that the users will
>>>>> use pointers anyway, so it's their unsafe responsiblity to guarantee
>>>>> that they don't use an invalid pointer?
>>>>>
>>>>> Also I assume you mean the others have refcounts to the page *before* an
>>>>> `Owned<Page>` is created, right? Because if we really have a use case
>>>>> where we want to have multiple users of a page after `Owned<Page>`
>>>>> created, we should better provide a `Owned<Page>` to `ARef<Page>`
>>>>> function.
>>>>
>>>> The __free_pages function just decrements a refcount. If there are
>>>> other references to it, it's not actually freed.
>>>>
>>>
>>> Then why don't we use page_put() there? ;-) And instead of
>>> `Owned<Page>`, we can wrap the kernel::page as `ARef<Page>`, no?
>>
>> I don't think there's a function called page_put?
>
> Sorry I confused myself. It's because it's called put_page.
>
How do I proceed with this? Should we use the page's reference count to
decide when to free the allocation and use put_page() instead of
__free_pages() in Page::Drop?.
In that case, there would be no need for `Ownable`, right? As we could
just return ARef<Page> in both vmalloc_to_page() case and in
Page::alloc_page(), letting the kernel handle ownership internally.
Regards,
Abdiel
next prev parent reply other threads:[~2024-11-01 13:38 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-22 22:44 [PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page") Abdiel Janulgue
2024-10-22 22:44 ` [PATCH v2 1/5] rust: types: add `Owned` type and `Ownable` trait Abdiel Janulgue
2024-10-23 8:10 ` Danilo Krummrich
2024-10-23 9:28 ` Alice Ryhl
2024-10-23 10:26 ` Abdiel Janulgue
2024-10-23 14:58 ` Boqun Feng
2024-10-23 17:52 ` Alice Ryhl
2024-10-23 18:07 ` Boqun Feng
2024-10-24 7:23 ` Alice Ryhl
2024-10-24 7:33 ` Alice Ryhl
2024-11-01 13:38 ` Abdiel Janulgue [this message]
2024-11-01 13:49 ` Alice Ryhl
2024-10-22 22:44 ` [PATCH v2 2/5] rust: page: Make ownership of the page pointer explicit Abdiel Janulgue
2024-10-22 22:44 ` [PATCH v2 3/5] rust: page: Extend support to vmalloc_to_page Abdiel Janulgue
2024-10-23 8:42 ` Danilo Krummrich
2024-10-23 9:03 ` Danilo Krummrich
2024-10-23 10:26 ` Abdiel Janulgue
2024-10-23 11:30 ` Danilo Krummrich
2024-10-22 22:44 ` [PATCH v2 4/5] rust: page: Add page_slice_to_page Abdiel Janulgue
2024-10-22 22:44 ` [PATCH v2 5/5] rust: firmware: implement `Ownable` for Firmware Abdiel Janulgue
2024-10-23 9:35 ` Danilo Krummrich
2024-10-23 9:45 ` Danilo Krummrich
2024-10-27 22:20 ` Boqun Feng
2024-10-28 13:37 ` Danilo Krummrich
2024-10-23 8:03 ` [PATCH v2 0/5] Introduce Owned type and Ownable trait (was: "rust: page: Add support for vmalloc_to_page") Danilo Krummrich
2024-10-23 9:51 ` Miguel Ojeda
2024-10-23 12:01 ` Danilo Krummrich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fe25c0a4-7e1b-45f3-b413-c52d033c7906@gmail.com \
--to=abdiel.janulgue@gmail.com \
--cc=airlied@redhat.com \
--cc=aliceryhl@google.com \
--cc=boqun.feng@gmail.com \
--cc=dakr@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=miguel.ojeda.sandonis@gmail.com \
--cc=rust-for-linux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome