Replacing all occurrences of `addr_of!(place)` and `addr_of_mut!(place)` with `&raw const place` and `&raw mut place` respectively.
This will allow us to reduce macro complexity, and improve consistency with existing reference syntax as `&raw const`, `&raw mut` are similar to `&`, `&mut` making it fit more naturally with other existing code.
Suggested-by: Benno Lossin benno.lossin@proton.me Link: https://github.com/Rust-for-Linux/linux/issues/1148 Signed-off-by: Antonio Hickey contact@antoniohickey.com --- rust/kernel/kunit.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..a17ef3b2e860 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -128,9 +128,9 @@ unsafe impl Sync for UnaryAssert {} unsafe { $crate::bindings::__kunit_do_failed_assertion( kunit_test, - core::ptr::addr_of!(LOCATION.0), + &raw const LOCATION.0, $crate::bindings::kunit_assert_type_KUNIT_ASSERTION, - core::ptr::addr_of!(ASSERTION.0.assert), + &raw const ASSERTION.0.assert, Some($crate::bindings::kunit_unary_assert_format), core::ptr::null(), );
On Sun, 16 Mar 2025 at 14:20, Antonio Hickey contact@byte-forge.io wrote:
Replacing all occurrences of `addr_of!(place)` and `addr_of_mut!(place)` with `&raw const place` and `&raw mut place` respectively.
This will allow us to reduce macro complexity, and improve consistency with existing reference syntax as `&raw const`, `&raw mut` are similar to `&`, `&mut` making it fit more naturally with other existing code.
Suggested-by: Benno Lossin benno.lossin@proton.me Link: https://github.com/Rust-for-Linux/linux/issues/1148 Signed-off-by: Antonio Hickey contact@antoniohickey.com
Thanks, Antonio.
So this looks fine, but it's also a bit annoying, as the remaining KUnit/Rust integration patches[1] were recently updated to use `addr_of_mut!()`, so either this patch, or those, will need updating.
In general, I think changes such as those in this series are going to get progressively more prone to conflicts as Rust is adopted by other subsystems, as the 'rust' tree won't be the only one carrying changes which could be affected. Maybe in the future it'd make sense to split these up into a series which enables the new feature, and only then put the warnings in place in the next version?
In the KUnit case in particular, since the patches haven't actually been merged yet, we have three options: - Merge this into rust-next, and send out a new version of the KUnit patches which depend on it, which then are also carried in rust-next, or delayed (again) to 6.16. I suspect given how close to the merge window we are, the delay is more likely. - Merge the KUnit changes as-is (either into rust-next or kselftest/kunit), and send out a new version of this series which also updates it (probably in 6.16, but maybe sooner if everything goes via rust-next). - Merge both, and accept that there'll be some clippy errors until a follow-up patch fixing them is sent and merged.
As someone with a vested interest in the KUnit changes, and at best a mild academic interest in the addr_of_muit!() deprecation, my preferences are with the latter two options. (I'd also, in general, whinge that the frequency of new Rust versions / warnings is high enough that it's taking a not insignificant portion of the limited time I have working on Rust things to understand and deal with the churn.)
Regardless, apart from the minor irritation of having to learn yet another new syntax, I have no objections to this patch in particular.
Reviewed-by: David Gow davidgow@google.com
Thanks (and sorry for the grumpiness: don't take it personally), -- David
[1]: https://lore.kernel.org/rust-for-linux/20250307090103.918788-1-davidgow@goog...
On Tue, Mar 18, 2025 at 04:02:15PM +0800, David Gow wrote:
On Sun, 16 Mar 2025 at 14:20, Antonio Hickey contact@byte-forge.io wrote:
Replacing all occurrences of `addr_of!(place)` and `addr_of_mut!(place)` with `&raw const place` and `&raw mut place` respectively.
This will allow us to reduce macro complexity, and improve consistency with existing reference syntax as `&raw const`, `&raw mut` are similar to `&`, `&mut` making it fit more naturally with other existing code.
Suggested-by: Benno Lossin benno.lossin@proton.me Link: https://github.com/Rust-for-Linux/linux/issues/1148 Signed-off-by: Antonio Hickey contact@antoniohickey.com
Thanks, Antonio.
So this looks fine, but it's also a bit annoying, as the remaining KUnit/Rust integration patches[1] were recently updated to use `addr_of_mut!()`, so either this patch, or those, will need updating.
In general, I think changes such as those in this series are going to get progressively more prone to conflicts as Rust is adopted by other subsystems, as the 'rust' tree won't be the only one carrying changes which could be affected. Maybe in the future it'd make sense to split these up into a series which enables the new feature, and only then put the warnings in place in the next version?
Hi David,
I understand your general frustration around this, but I do think it's better to have a higher frequency of change now while Rust adoption is still early than later on. I know you're looking at this in a more forward lense of the future, and I think you have a fair point that this can be more problematic as adoption increases.
In the KUnit case in particular, since the patches haven't actually been merged yet, we have three options:
- Merge this into rust-next, and send out a new version of the KUnit
patches which depend on it, which then are also carried in rust-next, or delayed (again) to 6.16. I suspect given how close to the merge window we are, the delay is more likely.
- Merge the KUnit changes as-is (either into rust-next or
kselftest/kunit), and send out a new version of this series which also updates it (probably in 6.16, but maybe sooner if everything goes via rust-next).
- Merge both, and accept that there'll be some clippy errors until a
follow-up patch fixing them is sent and merged.
I'm currently preparing to send out a new version of my patch right now. Depending on how the timing of our patches plays out, I'm not opposed to updating your patches KUnit changes within my patch set, as it's very much aligned with the scope of my patch set.
As someone with a vested interest in the KUnit changes, and at best a mild academic interest in the addr_of_muit!() deprecation, my preferences are with the latter two options. (I'd also, in general, whinge that the frequency of new Rust versions / warnings is high enough that it's taking a not insignificant portion of the limited time I have working on Rust things to understand and deal with the churn.)
Regardless, apart from the minor irritation of having to learn yet another new syntax, I have no objections to this patch in particular.
Reviewed-by: David Gow davidgow@google.com
Thanks (and sorry for the grumpiness: don't take it personally), -- David
Haha no problem, you have a fair point.
Thanks, Antonio
On Tue, Mar 18, 2025 at 9:02 AM David Gow davidgow@google.com wrote:
In general, I think changes such as those in this series are going to get progressively more prone to conflicts as Rust is adopted by other subsystems, as the 'rust' tree won't be the only one carrying changes which could be affected. Maybe in the future it'd make sense to split these up into a series which enables the new feature, and only then put the warnings in place in the next version?
+1, I had to do a two-cycle split for other things in the past already, e.g. for Gary's FFI series.
I agree that churn for this kind of change has a cost. For tree-wide series, it will become harder and harder over time, just like in C, and some changes and cleanups may take several cycles.
For Clippy lints, I think we have some extra flexibility. We still aim to keep everything Clippy-clean (and patches sent should be sent clean under the latest stable Rust version at least, and maintainers should enable Clippy in their test runs), but if something slips in a particular corner/config/arch that is not routinely tested, it is not the end of the world as long as it gets cleaned up.
Anyway, KUnit `#[test]`s are in -- I was not planning to merge this now anyway, it should be reviewed a bit more.
Thanks!
Cheers, Miguel
On Fri, 21 Mar 2025 at 07:16, Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Tue, Mar 18, 2025 at 9:02 AM David Gow davidgow@google.com wrote:
In general, I think changes such as those in this series are going to get progressively more prone to conflicts as Rust is adopted by other subsystems, as the 'rust' tree won't be the only one carrying changes which could be affected. Maybe in the future it'd make sense to split these up into a series which enables the new feature, and only then put the warnings in place in the next version?
+1, I had to do a two-cycle split for other things in the past already, e.g. for Gary's FFI series.
I agree that churn for this kind of change has a cost. For tree-wide series, it will become harder and harder over time, just like in C, and some changes and cleanups may take several cycles.
For Clippy lints, I think we have some extra flexibility. We still aim to keep everything Clippy-clean (and patches sent should be sent clean under the latest stable Rust version at least, and maintainers should enable Clippy in their test runs), but if something slips in a particular corner/config/arch that is not routinely tested, it is not the end of the world as long as it gets cleaned up.
Sounds like the right sort of compromise to me: if we aim to have things be clean on the branches they're applied to, we can clean up any warnings which arise as a result of merging afterwards.
Anyway, KUnit `#[test]`s are in -- I was not planning to merge this now anyway, it should be reviewed a bit more.
Excellent! I'll make sure to review the new version of the patch when it's rebased.
Cheers, -- David
On Fri, Mar 21, 2025 at 10:28:06AM +0800, David Gow wrote: [...]
Anyway, KUnit `#[test]`s are in -- I was not planning to merge this now anyway, it should be reviewed a bit more.
I agree this whole series should wait a bit, but do we want to merge patch #1 as early as possible (maybe right after v6.15-rc1), so that new code can switch to &raw since that's the direction anyway?
Regards, Boqun
Excellent! I'll make sure to review the new version of the patch when it's rebased.
Cheers, -- David
On Fri, Mar 21, 2025 at 6:06 PM Boqun Feng boqun.feng@gmail.com wrote:
I agree this whole series should wait a bit, but do we want to merge patch #1 as early as possible (maybe right after v6.15-rc1), so that new code can switch to &raw since that's the direction anyway?
Sounds like a good idea to me.
Cheers, Miguel
On Fri, Mar 21, 2025 at 10:06:03AM -0700, Boqun Feng wrote:
On Fri, Mar 21, 2025 at 10:28:06AM +0800, David Gow wrote: [...]
Anyway, KUnit `#[test]`s are in -- I was not planning to merge this now anyway, it should be reviewed a bit more.
I agree this whole series should wait a bit, but do we want to merge patch #1 as early as possible (maybe right after v6.15-rc1), so that new code can switch to &raw since that's the direction anyway?
This would make the most sense to me, it would keep things clippy clean while also allowing new patches to use the feature. This would also potentially help reduce the amount of refactoring my patch series will have to do as it's delayed.
Thanks, Antonio
Regards, Boqun
Excellent! I'll make sure to review the new version of the patch when it's rebased.
Cheers, -- David
linux-kselftest-mirror@lists.linaro.org
-
Antonio Hickey -
Antonio Hickey -
Boqun Feng -
David Gow -
Miguel Ojeda