[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bH_zaB8RmZZW2QrGBx1ud7-YfKmh6QvTU0jYKC0ns7jjoDkCWYnW3u1qX_YrN5P0VwsZGd7U5r8p-7DxH7pb4-6UUE0htwTkFNdDIYZb4os=@proton.me>
Date: Thu, 30 Nov 2023 17:42:06 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Wedson Almeida Filho <wedsonaf@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Andreas Hindborg <a.hindborg@...sung.com>,
Peter Zijlstra <peterz@...radead.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>,
Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joel@...lfernandes.org>,
Carlos Llamas <cmllamas@...gle.com>,
Suren Baghdasaryan <surenb@...gle.com>,
Dan Williams <dan.j.williams@...el.com>,
Kees Cook <keescook@...omium.org>,
Matthew Wilcox <willy@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
Daniel Xu <dxu@...uu.xyz>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 7/7] rust: file: add abstraction for `poll_table`
On 11/29/23 14:12, Alice Ryhl wrote:
> diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
> index 578ee307093f..35576678c993 100644
> --- a/rust/kernel/file.rs
> +++ b/rust/kernel/file.rs
> @@ -14,6 +14,9 @@
> use alloc::boxed::Box;
> use core::{alloc::AllocError, marker::PhantomData, mem, ptr};
>
> +mod poll_table;
> +pub use self::poll_table::{PollCondVar, PollTable};
I think it makes more sense to put it under `rust/kernel/sync/`.
> + fn get_qproc(&self) -> bindings::poll_queue_proc {
> + let ptr = self.0.get();
> + // SAFETY: The `ptr` is valid because it originates from a reference, and the `_qproc`
> + // field is not modified concurrently with this call.
What ensures this? Maybe use a type invariant?
> + unsafe { (*ptr)._qproc }
> + }
[...]
> +impl PollCondVar {
> + /// Constructs a new condvar initialiser.
> + #[allow(clippy::new_ret_no_self)]
This is no longer needed, as Gary fixed this, see [1].
[1]: https://github.com/rust-lang/rust-clippy/issues/7344
> + pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> + pin_init!(Self {
> + inner <- CondVar::new(name, key),
> + })
> + }
> +}
> +
> +// Make the `CondVar` methods callable on `PollCondVar`.
> +impl Deref for PollCondVar {
> + type Target = CondVar;
> +
> + fn deref(&self) -> &CondVar {
> + &self.inner
> + }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for PollCondVar {
> + fn drop(self: Pin<&mut Self>) {
> + // Clear anything registered using `register_wait`.
> + self.inner.notify(1, bindings::POLLHUP | bindings::POLLFREE);
Isn't notifying only a single thread problematic, since a user could
misuse the `PollCondVar` (since all functions of `CondVar` are also
accessible) and also `.wait()` on the condvar? When dropping a
`PollCondVar` it might notify only the user `.wait()`, but not the
`PollTable`. Or am I missing something?
--
Cheers,
Benno
> + // Wait for epoll items to be properly removed.
> + //
> + // SAFETY: Just an FFI call.
> + unsafe { bindings::synchronize_rcu() };
> + }
> +}
Powered by blists - more mailing lists