mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
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



  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