All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Gary Guo" <gary@garyguo.net>
Cc: "Danilo Krummrich" <dakr@kernel.org>,
	"Abdiel Janulgue" <abdiel.janulgue@gmail.com>,
	"Daniel Almeida" <daniel.almeida@collabora.com>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun@kernel.org>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"David Airlie" <airlied@gmail.com>,
	"Simona Vetter" <simona@ffwll.ch>,
	driver-core@lists.linux.dev, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/5] rust: ptr: add panicking index projection variant
Date: Wed, 29 Apr 2026 20:22:20 +0900	[thread overview]
Message-ID: <DI5LLN2V3XCS.34H4CG99N4MPA@nvidia.com> (raw)
In-Reply-To: <20260415-projection-syntax-rework-v1-1-450723cb3727@garyguo.net>

On Thu Apr 16, 2026 at 4:57 AM JST, Gary Guo wrote:
> There have been a few cases where the programmer knows that the indices are
> in bounds but compiler cannot deduce that. This is also
> compiler-version-dependent, so using build indexing here can be
> problematic. On the other hand, it is also not ideal to use the fallible
> variant, as it adds error handling path that is never hit.
>
> Add a new panicking index projection for this scenario. Like all panicking
> operations, this should be used carefully only in cases where the user
> knows the index is going to be in bounds, and panicking would indicate
> something is catastrophically wrong.
>
> To signify this, require users to explicitly denote the type of index being
> used. The existing two types of index projections also gain the keyworded
> version, which will be the recommended way going forward.
>
> The keyworded syntax also paves the way of perhaps adding more flavors in
> the future, e.g. `unsafe` index projection. However, unless the code is
> extremely performance sensitive and bounds checking cannot be tolerated,
> panicking variant is safer and should be preferred, so it will be left to
> future when demand arises.

Review nit (no need to respin for this): this patch would probably be
easier to review if the renaming of `index` to `build_index` was split
into its own patch. It would reduce the diff, while also removing the
burden of having to keep in mind that `index` means a different thing
depending on whether the line is removed or added. I've found it a bit
difficult to keep track of this.

>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  rust/kernel/dma.rs            |  3 ++
>  rust/kernel/ptr/projection.rs | 98 +++++++++++++++++++++++++++++++++++--------
>  2 files changed, 84 insertions(+), 17 deletions(-)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 4995ee5dc689..3e4d44749aaf 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -1207,6 +1207,9 @@ macro_rules! dma_write {
>      (@parse [$dma:expr] [$($proj:tt)*] [.$field:tt $($rest:tt)*]) => {
>          $crate::dma_write!(@parse [$dma] [$($proj)* .$field] [$($rest)*])
>      };
> +    (@parse [$dma:expr] [$($proj:tt)*] [[$flavor:ident: $index:expr] $($rest:tt)*]) => {
> +        $crate::dma_write!(@parse [$dma] [$($proj)* [$flavor: $index]] [$($rest)*])
> +    };
>      (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr]? $($rest:tt)*]) => {
>          $crate::dma_write!(@parse [$dma] [$($proj)* [$index]?] [$($rest)*])
>      };
> diff --git a/rust/kernel/ptr/projection.rs b/rust/kernel/ptr/projection.rs
> index 140ea8e21617..845811795393 100644
> --- a/rust/kernel/ptr/projection.rs
> +++ b/rust/kernel/ptr/projection.rs
> @@ -26,14 +26,14 @@ fn from(_: OutOfBound) -> Self {
>  ///
>  /// # Safety
>  ///
> -/// The implementation of `index` and `get` (if [`Some`] is returned) must ensure that, if provided
> -/// input pointer `slice` and returned pointer `output`, then:
> +/// The implementation of `index`, `build_index` and `get` (if [`Some`] is returned) must ensure
> +/// that, if provided input pointer `slice` and returned pointer `output`, then:
>  /// - `output` has the same provenance as `slice`;
>  /// - `output.byte_offset_from(slice)` is between 0 to
>  ///   `KnownSize::size(slice) - KnownSize::size(output)`.
>  ///
> -/// This means that if the input pointer is valid, then pointer returned by `get` or `index` is
> -/// also valid.
> +/// This means that if the input pointer is valid, then pointer returned by `get`, `index` or
> +/// `build_index` is also valid.
>  #[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")]
>  #[doc(hidden)]
>  pub unsafe trait ProjectIndex<T: ?Sized>: Sized {
> @@ -42,9 +42,12 @@ pub unsafe trait ProjectIndex<T: ?Sized>: Sized {
>      /// Returns an index-projected pointer, if in bounds.
>      fn get(self, slice: *mut T) -> Option<*mut Self::Output>;
>  
> +    /// Returns an index-projected pointer; panic if out of bounds.
> +    fn index(self, slice: *mut T) -> *mut Self::Output;

This looks like this could have a default implementation:

    fn index(self, slice: *mut T) -> *mut Self::Output {
        Self::get(self, slice).unwrap()
    }

I'm sure there is a good reason for now having this though, so at least
for my education: why not? :)

In any case,

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

WARNING: multiple messages have this Message-ID (diff)
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Gary Guo" <gary@garyguo.net>
Cc: "Danilo Krummrich" <dakr@kernel.org>,
	"Abdiel Janulgue" <abdiel.janulgue@gmail.com>,
	"Daniel Almeida" <daniel.almeida@collabora.com>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun@kernel.org>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Simona Vetter" <simona@ffwll.ch>,
	driver-core@lists.linux.dev, rust-for-linux@vger.kernel.org,
	linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/5] rust: ptr: add panicking index projection variant
Date: Wed, 29 Apr 2026 20:22:20 +0900	[thread overview]
Message-ID: <DI5LLN2V3XCS.34H4CG99N4MPA@nvidia.com> (raw)
In-Reply-To: <20260415-projection-syntax-rework-v1-1-450723cb3727@garyguo.net>

On Thu Apr 16, 2026 at 4:57 AM JST, Gary Guo wrote:
> There have been a few cases where the programmer knows that the indices are
> in bounds but compiler cannot deduce that. This is also
> compiler-version-dependent, so using build indexing here can be
> problematic. On the other hand, it is also not ideal to use the fallible
> variant, as it adds error handling path that is never hit.
>
> Add a new panicking index projection for this scenario. Like all panicking
> operations, this should be used carefully only in cases where the user
> knows the index is going to be in bounds, and panicking would indicate
> something is catastrophically wrong.
>
> To signify this, require users to explicitly denote the type of index being
> used. The existing two types of index projections also gain the keyworded
> version, which will be the recommended way going forward.
>
> The keyworded syntax also paves the way of perhaps adding more flavors in
> the future, e.g. `unsafe` index projection. However, unless the code is
> extremely performance sensitive and bounds checking cannot be tolerated,
> panicking variant is safer and should be preferred, so it will be left to
> future when demand arises.

Review nit (no need to respin for this): this patch would probably be
easier to review if the renaming of `index` to `build_index` was split
into its own patch. It would reduce the diff, while also removing the
burden of having to keep in mind that `index` means a different thing
depending on whether the line is removed or added. I've found it a bit
difficult to keep track of this.

>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  rust/kernel/dma.rs            |  3 ++
>  rust/kernel/ptr/projection.rs | 98 +++++++++++++++++++++++++++++++++++--------
>  2 files changed, 84 insertions(+), 17 deletions(-)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 4995ee5dc689..3e4d44749aaf 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -1207,6 +1207,9 @@ macro_rules! dma_write {
>      (@parse [$dma:expr] [$($proj:tt)*] [.$field:tt $($rest:tt)*]) => {
>          $crate::dma_write!(@parse [$dma] [$($proj)* .$field] [$($rest)*])
>      };
> +    (@parse [$dma:expr] [$($proj:tt)*] [[$flavor:ident: $index:expr] $($rest:tt)*]) => {
> +        $crate::dma_write!(@parse [$dma] [$($proj)* [$flavor: $index]] [$($rest)*])
> +    };
>      (@parse [$dma:expr] [$($proj:tt)*] [[$index:expr]? $($rest:tt)*]) => {
>          $crate::dma_write!(@parse [$dma] [$($proj)* [$index]?] [$($rest)*])
>      };
> diff --git a/rust/kernel/ptr/projection.rs b/rust/kernel/ptr/projection.rs
> index 140ea8e21617..845811795393 100644
> --- a/rust/kernel/ptr/projection.rs
> +++ b/rust/kernel/ptr/projection.rs
> @@ -26,14 +26,14 @@ fn from(_: OutOfBound) -> Self {
>  ///
>  /// # Safety
>  ///
> -/// The implementation of `index` and `get` (if [`Some`] is returned) must ensure that, if provided
> -/// input pointer `slice` and returned pointer `output`, then:
> +/// The implementation of `index`, `build_index` and `get` (if [`Some`] is returned) must ensure
> +/// that, if provided input pointer `slice` and returned pointer `output`, then:
>  /// - `output` has the same provenance as `slice`;
>  /// - `output.byte_offset_from(slice)` is between 0 to
>  ///   `KnownSize::size(slice) - KnownSize::size(output)`.
>  ///
> -/// This means that if the input pointer is valid, then pointer returned by `get` or `index` is
> -/// also valid.
> +/// This means that if the input pointer is valid, then pointer returned by `get`, `index` or
> +/// `build_index` is also valid.
>  #[diagnostic::on_unimplemented(message = "`{Self}` cannot be used to index `{T}`")]
>  #[doc(hidden)]
>  pub unsafe trait ProjectIndex<T: ?Sized>: Sized {
> @@ -42,9 +42,12 @@ pub unsafe trait ProjectIndex<T: ?Sized>: Sized {
>      /// Returns an index-projected pointer, if in bounds.
>      fn get(self, slice: *mut T) -> Option<*mut Self::Output>;
>  
> +    /// Returns an index-projected pointer; panic if out of bounds.
> +    fn index(self, slice: *mut T) -> *mut Self::Output;

This looks like this could have a default implementation:

    fn index(self, slice: *mut T) -> *mut Self::Output {
        Self::get(self, slice).unwrap()
    }

I'm sure there is a good reason for now having this though, so at least
for my education: why not? :)

In any case,

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

  parent reply	other threads:[~2026-04-29 11:22 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-15 19:57 [PATCH 0/5] Rework index projection syntax Gary Guo
2026-04-15 19:57 ` [PATCH 1/5] rust: ptr: add panicking index projection variant Gary Guo
2026-04-16  7:07   ` Alice Ryhl
2026-04-16  7:07     ` Alice Ryhl
2026-04-27 11:24   ` Andreas Hindborg
2026-04-27 12:45     ` Gary Guo
2026-04-27 12:45       ` Gary Guo
2026-04-29 11:22   ` Alexandre Courbot [this message]
2026-04-29 11:22     ` Alexandre Courbot
2026-04-29 11:29     ` Gary Guo
2026-04-29 11:29       ` Gary Guo
2026-04-30 11:23       ` Alexandre Courbot
2026-04-30 11:23         ` Alexandre Courbot
2026-04-30 11:54         ` Gary Guo
2026-04-30 11:54           ` Gary Guo
2026-04-15 19:57 ` [PATCH 2/5] rust: dma: update to keyworded index projection syntax Gary Guo
2026-04-16  7:07   ` Alice Ryhl
2026-04-16  7:07     ` Alice Ryhl
2026-04-27 11:25   ` Andreas Hindborg
2026-04-29 11:22   ` Alexandre Courbot
2026-04-29 11:22     ` Alexandre Courbot
2026-04-15 19:57 ` [PATCH 3/5] gpu: nova-core: convert to keyworded " Gary Guo
2026-04-16  7:08   ` Alice Ryhl
2026-04-16  7:08     ` Alice Ryhl
2026-04-29 11:22   ` Alexandre Courbot
2026-04-29 11:22     ` Alexandre Courbot
2026-04-15 19:57 ` [PATCH 4/5] gpu: nova-core: use pointer projection for command queue code Gary Guo
2026-04-16  7:14   ` Alice Ryhl
2026-04-16  7:14     ` Alice Ryhl
2026-04-29 11:23   ` Alexandre Courbot
2026-04-29 11:23     ` Alexandre Courbot
2026-04-15 19:57 ` [PATCH 5/5] rust: ptr: remove implicit index projection syntax Gary Guo
2026-04-16  7:14   ` Alice Ryhl
2026-04-16  7:14     ` Alice Ryhl
2026-04-27 11:28   ` Andreas Hindborg
2026-04-29 11:22   ` Alexandre Courbot
2026-04-29 11:22     ` Alexandre Courbot

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=DI5LLN2V3XCS.34H4CG99N4MPA@nvidia.com \
    --to=acourbot@nvidia.com \
    --cc=a.hindborg@kernel.org \
    --cc=abdiel.janulgue@gmail.com \
    --cc=airlied@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=dakr@kernel.org \
    --cc=daniel.almeida@collabora.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=driver-core@lists.linux.dev \
    --cc=gary@garyguo.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=nouveau@lists.freedesktop.org \
    --cc=ojeda@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=simona@ffwll.ch \
    --cc=tmgross@umich.edu \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.