From: Andreas Hindborg <hidden> Date: 2024-08-15 07:49:45
From: Andreas Hindborg <redacted>
The rust block device bindings include a wrong use of lock class key. This
causes a WARN trace when lockdep is enabled and a `GenDisk` is constructed.
This series fixes the issue by using a static lock class key. To do this, the
series first fixes the rust build system to correctly export rust statics from
the bss segment.
Andreas Hindborg (2):
rust: fix export of bss symbols
rust: block: fix wrong usage of lockdep API
rust/Makefile | 2 +-
rust/kernel/block/mq/gen_disk.rs | 6 ++----
2 files changed, 3 insertions(+), 5 deletions(-)
base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
--
2.46.0
From: Andreas Hindborg <hidden> Date: 2024-08-15 07:49:49
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
---
rust/kernel/block/mq/gen_disk.rs | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
@@ -6,7 +6,7 @@//! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)usecrate::block::mq::{raw_writer::RawWriter,Operations,TagSet};-usecrate::error;+usecrate::{error,static_lock_class};usecrate::{bindings,error::from_err_ptr,error::Result,sync::Arc};usecore::fmt::{self,Write};
@@ -93,8 +93,6 @@ pub fn build<T: Operations>(name:fmt::Arguments<'_>,tagset:Arc<TagSet<T>>,)->Result<GenDisk<T>>{-letlock_class_key=crate::sync::LockClassKey::new();-// SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.letmutlim:bindings::queue_limits=unsafe{core::mem::zeroed()};
From: Alice Ryhl <aliceryhl@google.com> Date: 2024-08-15 08:05:11
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
Tested-by: Dirk Behme <redacted>
Many thanks! :)
Dirk
@@ -6,7 +6,7 @@//! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)usecrate::block::mq::{raw_writer::RawWriter,Operations,TagSet};-usecrate::error;+usecrate::{error,static_lock_class};usecrate::{bindings,error::from_err_ptr,error::Result,sync::Arc};usecore::fmt::{self,Write};
@@ -93,8 +93,6 @@ pub fn build<T: Operations>(name:fmt::Arguments<'_>,tagset:Arc<TagSet<T>>,)->Result<GenDisk<T>>{-letlock_class_key=crate::sync::LockClassKey::new();-// SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.letmutlim:bindings::queue_limits=unsafe{core::mem::zeroed()};
--
======================================================================
Dirk Behme Robert Bosch Car Multimedia GmbH
CM/ESO2
Phone: +49 5121 49-3274 Dirk Behme
Fax: +49 711 811 5053274 PO Box 77 77 77
mailto:dirk.behme@de.bosch.com D-31132 Hildesheim - Germany
Bosch Group, Car Multimedia (CM)
Engineering SW Operating Systems 2 (ESO2)
Robert Bosch Car Multimedia GmbH - Ein Unternehmen der Bosch Gruppe
Sitz: Hildesheim
Registergericht: Amtsgericht Hildesheim HRB 201334
Aufsichtsratsvorsitzender: Dr. Dirk Hoheisel
Geschäftsführung: Dr. Steffen Berns;
Dr. Sven Ost, Jörg Pollak, Dr. Walter Schirm
======================================================================
On Thu, 15 Aug 2024 07:49:37 +0000, Andreas Hindborg wrote:
The rust block device bindings include a wrong use of lock class key. This
causes a WARN trace when lockdep is enabled and a `GenDisk` is constructed.
This series fixes the issue by using a static lock class key. To do this, the
series first fixes the rust build system to correctly export rust statics from
the bss segment.
[...]
Applied, thanks!
[2/2] rust: block: fix wrong usage of lockdep API
commit: d28b514ea3ae15274a4d70422ecc873bf6258e77
Best regards,
--
Jens Axboe
From: Miguel Ojeda <hidden> Date: 2024-08-15 15:31:38
Hi Jens,
On Thu, Aug 15, 2024 at 4:22 PM Jens Axboe [off-list ref] wrote:
Applied, thanks!
[2/2] rust: block: fix wrong usage of lockdep API
commit: d28b514ea3ae15274a4d70422ecc873bf6258e77
If you picked 2/2 only, it requires the former as far as I understand.
If you want to pick both:
Acked-by: Miguel Ojeda <ojeda@kernel.org>
Otherwise, I am happy to take them too.
Thanks!
Cheers,
Miguel
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
So `LockClassKey::new` doesn't initialize the `lock_class_key` and is
also movable. I think in this case we either just overlooked it or
thought that the C side would initialize it.
For those people that know about this, are there APIs that initialize
`lock_class_key` themselves? (ie not a function to initialize a lock
class key, but rather an API like `__blk_mq_alloc_disk`)
Because if it is usually expected that the class key is already
initialized, then I think we should change our abstraction.
Additionally, I think that it needs to be pinned, since it contains an
`struct hlist_node` (I might be wrong on this, but that looks and sounds
like an intrusive linked list).
Also the `new` function is probably prone for misuse, since it will
create a new lock class key every time it is run. But as I learned in
[1], the more common use-case is a single lock class key for several
locks. Therefore it might be a good idea to at least rename it to
`new_dynamic` or similar and add appropriate documentation pointing to
`static_lock_class!`.
[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.E2.9C.94.206.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning/near/460074755
---
Cheers,
Benno
From: Gary Guo <gary@garyguo.net> Date: 2024-08-15 19:07:44
On Thu, 15 Aug 2024 10:04:56 +0200
Alice Ryhl [off-list ref] wrote:
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
I agree. The API that we current have is designed without much
consideration into dynamically allocated keys, and we use `&'static
LockClassKey` in a lot of kernel crate APIs.
This arguably is wrong, because presence of `&'static LockClassKey`
doesn't mean the key is static. If we do a
`Box::leak(Box::new(LockClassKey::new()))`, then this is a `&'static
LockClassKey`, but lockdep wouldn't consider this as a static object.
Maybe we should make the `new` function unsafe.
For the patch itself:
Reviewed-by: Gary Guo <gary@garyguo.net>
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
So `LockClassKey::new` doesn't initialize the `lock_class_key` and is
also movable. I think in this case we either just overlooked it or
thought that the C side would initialize it.
For those people that know about this, are there APIs that initialize
`lock_class_key` themselves? (ie not a function to initialize a lock
class key, but rather an API like `__blk_mq_alloc_disk`)
Because if it is usually expected that the class key is already
initialized, then I think we should change our abstraction.
Sorry, I got confused, this has nothing to do with initialization.
---
Cheers,
Benno
Additionally, I think that it needs to be pinned, since it contains an
`struct hlist_node` (I might be wrong on this, but that looks and sounds
like an intrusive linked list).
Also the `new` function is probably prone for misuse, since it will
create a new lock class key every time it is run. But as I learned in
[1], the more common use-case is a single lock class key for several
locks. Therefore it might be a good idea to at least rename it to
`new_dynamic` or similar and add appropriate documentation pointing to
`static_lock_class!`.
[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.E2.9C.94.206.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning/near/460074755
---
Cheers,
Benno
On Thu, Aug 15, 2024 at 08:07:38PM +0100, Gary Guo wrote:
On Thu, 15 Aug 2024 10:04:56 +0200
Alice Ryhl [off-list ref] wrote:
quoted
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
I agree. The API that we current have is designed without much
consideration into dynamically allocated keys, and we use `&'static
LockClassKey` in a lot of kernel crate APIs.
This arguably is wrong, because presence of `&'static LockClassKey`
doesn't mean the key is static. If we do a
`Box::leak(Box::new(LockClassKey::new()))`, then this is a `&'static
LockClassKey`, but lockdep wouldn't consider this as a static object.
Maybe we should make the `new` function unsafe.
I think a more proper fix is to make LockClassKey pin-init, for
dynamically allocated LockClassKey, we just use lockdep_register_key()
as the initializer and lockdep_unregister_key() as the desconstructor.
And instead of a `&'static LockClassKey`, we should use `Pin<&'static
LockClassKey>` to pass a lock class key. Of course we will need some
special treatment on static allocated keys (e.g. assume they are
initialized since lockdep doesn't require initialization for them).
Pin initializer:
impl LockClassKey {
pub fn new() -> impl PinInit<Self> {
pin_init!(Self {
inner <- Opaque::ffi_init(|slot| { lockdep_register_key(slot) })
})
}
}
LockClassKey::new_uninit() for `static_lock_class!`:
impl LockClassKey {
pub const fn new_uninit() -> MaybeUninit<Self> {
....
}
}
and the new `static_lock_class!`:
macro_rules! static_lock_class {
() => {{
static CLASS: MaybeUninit<$crate::sync::LockClassKey> = $crate::sync::LockClassKey::new_uninit();
// SAFETY: `CLASS` is pinned because it's static
// allocated. And it's OK to assume it's initialized
// because lockdep support uninitialized static
// allocated key.
unsafe { Pin::new_unchecked(CLASS.assume_init_ref()) }
}};
}
Thoughts?
Regards,
Boqun
For the patch itself:
Reviewed-by: Gary Guo <gary@garyguo.net>
On Thu, Aug 15, 2024 at 07:15:43PM +0000, Benno Lossin wrote:
On 15.08.24 21:05, Benno Lossin wrote:
quoted
On 15.08.24 10:04, Alice Ryhl wrote:
quoted
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
So `LockClassKey::new` doesn't initialize the `lock_class_key` and is
also movable. I think in this case we either just overlooked it or
thought that the C side would initialize it.
For those people that know about this, are there APIs that initialize
`lock_class_key` themselves? (ie not a function to initialize a lock
class key, but rather an API like `__blk_mq_alloc_disk`)
Because if it is usually expected that the class key is already
initialized, then I think we should change our abstraction.
Sorry, I got confused, this has nothing to do with initialization.
For static allocated key, no initialization is needed, for dynamic
allocated key, lockdep_register_key() will need to be called before
using the key.
Regards,
Boqun
---
Cheers,
Benno
quoted
Additionally, I think that it needs to be pinned, since it contains an
`struct hlist_node` (I might be wrong on this, but that looks and sounds
like an intrusive linked list).
Also the `new` function is probably prone for misuse, since it will
create a new lock class key every time it is run. But as I learned in
[1], the more common use-case is a single lock class key for several
locks. Therefore it might be a good idea to at least rename it to
`new_dynamic` or similar and add appropriate documentation pointing to
`static_lock_class!`.
[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.E2.9C.94.206.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning/near/460074755
---
Cheers,
Benno
From: Gary Guo <gary@garyguo.net> Date: 2024-08-15 21:42:38
On Thu, 15 Aug 2024 14:32:28 -0700
Boqun Feng [off-list ref] wrote:
On Thu, Aug 15, 2024 at 08:07:38PM +0100, Gary Guo wrote:
quoted
On Thu, 15 Aug 2024 10:04:56 +0200
Alice Ryhl [off-list ref] wrote:
quoted
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
I agree. The API that we current have is designed without much
consideration into dynamically allocated keys, and we use `&'static
LockClassKey` in a lot of kernel crate APIs.
This arguably is wrong, because presence of `&'static LockClassKey`
doesn't mean the key is static. If we do a
`Box::leak(Box::new(LockClassKey::new()))`, then this is a `&'static
LockClassKey`, but lockdep wouldn't consider this as a static object.
Maybe we should make the `new` function unsafe.
I think a more proper fix is to make LockClassKey pin-init, for
dynamically allocated LockClassKey, we just use lockdep_register_key()
as the initializer and lockdep_unregister_key() as the desconstructor.
And instead of a `&'static LockClassKey`, we should use `Pin<&'static
LockClassKey>` to pass a lock class key. Of course we will need some
special treatment on static allocated keys (e.g. assume they are
initialized since lockdep doesn't require initialization for them).
Pin initializer:
impl LockClassKey {
pub fn new() -> impl PinInit<Self> {
pin_init!(Self {
inner <- Opaque::ffi_init(|slot| { lockdep_register_key(slot) })
})
}
}
LockClassKey::new_uninit() for `static_lock_class!`:
impl LockClassKey {
pub const fn new_uninit() -> MaybeUninit<Self> {
....
}
}
and the new `static_lock_class!`:
macro_rules! static_lock_class {
() => {{
static CLASS: MaybeUninit<$crate::sync::LockClassKey> = $crate::sync::LockClassKey::new_uninit();
nit: this could just be `MaybeUninit::uninit()`
// SAFETY: `CLASS` is pinned because it's static
// allocated. And it's OK to assume it's initialized
// because lockdep support uninitialized static
// allocated key.
unsafe { Pin::new_unchecked(CLASS.assume_init_ref()) }
nit: this could be `Pin::from_static(unsafe { CLASS.assume_init_ref() })`
}};
}
Thoughts?
I think this design looks good. I suggested adding unsafe as a quick
way to address the pontential misuse, when we have no user for
dynamically allocated keys.
Best,
Gary
On Thu, 15 Aug 2024 14:32:28 -0700
Boqun Feng [off-list ref] wrote:
quoted
On Thu, Aug 15, 2024 at 08:07:38PM +0100, Gary Guo wrote:
quoted
On Thu, 15 Aug 2024 10:04:56 +0200
Alice Ryhl [off-list ref] wrote:
quoted
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
quoted
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
I agree. The API that we current have is designed without much
consideration into dynamically allocated keys, and we use `&'static
LockClassKey` in a lot of kernel crate APIs.
This arguably is wrong, because presence of `&'static LockClassKey`
doesn't mean the key is static. If we do a
`Box::leak(Box::new(LockClassKey::new()))`, then this is a `&'static
LockClassKey`, but lockdep wouldn't consider this as a static object.
Maybe we should make the `new` function unsafe.
I think a more proper fix is to make LockClassKey pin-init, for
dynamically allocated LockClassKey, we just use lockdep_register_key()
as the initializer and lockdep_unregister_key() as the desconstructor.
And instead of a `&'static LockClassKey`, we should use `Pin<&'static
LockClassKey>` to pass a lock class key. Of course we will need some
special treatment on static allocated keys (e.g. assume they are
initialized since lockdep doesn't require initialization for them).
Pin initializer:
impl LockClassKey {
pub fn new() -> impl PinInit<Self> {
pin_init!(Self {
inner <- Opaque::ffi_init(|slot| { lockdep_register_key(slot) })
})
}
}
LockClassKey::new_uninit() for `static_lock_class!`:
impl LockClassKey {
pub const fn new_uninit() -> MaybeUninit<Self> {
We don't need to wrap it in `MaybeUninit`, since it already is
containing an `Opaque`. But I think we don't need to expose this
function at all, see below.
quoted
....
}
}
and the new `static_lock_class!`:
macro_rules! static_lock_class {
() => {{
static CLASS: MaybeUninit<$crate::sync::LockClassKey> = $crate::sync::LockClassKey::new_uninit();
() => {{
// SAFETY: `LockClassKey` contains a single field of type `Opaque` and thus an uninitialized
// value is valid.
static CLASS: $crate::sync::LockClassKey = unsafe {
::core::mem::MaybeUninit::uninit().assume_init()
};
Pin::from_static(&CLASS)
}};
That way users can either create a static class, or a dynamic one via
`new_dynmaic` (I think we should rename it while we're at it), which is
always registered.
nit: this could just be `MaybeUninit::uninit()`
quoted
// SAFETY: `CLASS` is pinned because it's static
// allocated. And it's OK to assume it's initialized
// because lockdep support uninitialized static
// allocated key.
unsafe { Pin::new_unchecked(CLASS.assume_init_ref()) }
nit: this could be `Pin::from_static(unsafe { CLASS.assume_init_ref() })`
quoted
}};
}
Thoughts?
I think this design looks good. I suggested adding unsafe as a quick
way to address the pontential misuse, when we have no user for
dynamically allocated keys.
I think we should do it properly, since the solution seems easy.
---
Cheers,
Benno
From: Andreas Hindborg <redacted>
When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
class key without registering the key. This is incorrect use of the API,
which causes a `WARN` trace. This patch fixes the issue by using a static
lock class key, which is more appropriate for the situation anyway.
Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
Reported-by: "Behme Dirk (XC-CP/ESB5)" <redacted>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
Signed-off-by: Andreas Hindborg <redacted>
---
rust/kernel/block/mq/gen_disk.rs | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Miguel Ojeda <hidden> Date: 2024-08-21 10:50:57
On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg [off-list ref] wrote:
From: Andreas Hindborg <redacted>
The rust block device bindings include a wrong use of lock class key. This
causes a WARN trace when lockdep is enabled and a `GenDisk` is constructed.
This series fixes the issue by using a static lock class key. To do this, the
series first fixes the rust build system to correctly export rust statics from
the bss segment.
Applied to `rust-fixes` -- thanks everyone!
(I am sending the notice twice for this series, since somehow the
email threads got split into two in Lore, which also broke `b4`)
[ Applied `rustfmt` and reworded slightly. - Miguel ]
Cheers,
Miguel
From: Miguel Ojeda <hidden> Date: 2024-08-21 11:37:00
On Wed, Aug 21, 2024 at 12:50 PM Miguel Ojeda
[off-list ref] wrote:
[ Applied `rustfmt` and reworded slightly. - Miguel ]
Dirk noticed the Zulip link was broken due to a rename of the topic.
Normally I try to remember to convert them into message links (which
are permalinks) and to shorten them when they are too long too -- done
now.
For context, Zulip is gaining the ability to get topic permalinks (see
https://github.com/zulip/zulip/issues/21505 -- we gave them feedback a
year ago that it would be useful for kernel commits), though I think
it is not exposed through the UI yet.
Thanks Dirk!
Cheers,
Miguel
From: Miguel Ojeda <hidden> Date: 2024-08-21 11:54:49
On Wed, Aug 21, 2024 at 1:36 PM Miguel Ojeda
[off-list ref] wrote:
Dirk noticed the Zulip link was broken due to a rename of the topic.
Normally I try to remember to convert them into message links (which
are permalinks) and to shorten them when they are too long too -- done
now.
For context, Zulip is gaining the ability to get topic permalinks (see
https://github.com/zulip/zulip/issues/21505 -- we gave them feedback a
year ago that it would be useful for kernel commits), though I think
it is not exposed through the UI yet.