[RFC,00/11] rust: Implicit lock class creation & Arc Lockdep integration

  Begone, lock classes!

As discussed in meetings/etc, we would really like to support implicit
lock class creation for Rust code. Right now, lock classes are created
using macros and passed around (similar to C). Unfortunately, Rust
macros don't look like Rust functions, which means adding lockdep to a
type is a breaking API change. This makes Rust mutex creation rather
ugly, with the new_mutex!() macro and friends.

Implicit lock classes have to be unique per instantiation code site.
Notably, with Rust generics and monomorphization, this is not the same
as unique per generated code instance. If this weren't the case, we
could use inline functions and asm!() magic to try to create lock
classes that have the right uniqueness semantics. But that doesn't work,
since it would create too many lock classes for the same actual lock
creation in the source code.

But Rust does have one trick we can use: it can track the caller
location (as file:line:column), across multiple functions. This works
using an implicit argument that gets passed around, which is exactly the
thing we do for lock classes. The tricky bit is that, while the value of
these Location objects has the semantics we want (unique value per
source code location), there is no guarantee that they are deduplicated
in memory.

So we use a hash table, and map Location values to lock classes. Et
voila, implicit lock class support!

This lets us clean up the Mutex & co APIs and make them look a lot more
Rust-like, but it also means we can now throw Lockdep into more APIs
without breaking the API. And so we can pull a neat trick: adding
Lockdep support into Arc<T>. This catches cases where the Arc Drop
implementation could create a locking correctness violation only when
the reference count drops to 0 at that particular drop site, which is
otherwise not detectable unless that condition actually happens at
runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
to audit, this helps a lot.

For the initial RFC, this implements the new API only for Mutex. If this
looks good, I can extend it to CondVar & friends in the next version.
This series also folds in a few related minor dependencies / changes
(like the pin_init mutex stuff).

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
Asahi Lina (11):
      rust: types: Add Opaque::zeroed()
      rust: lock: Add Lock::pin_init()
      rust: Use absolute paths to build Rust objects
      rust: siphash: Add a simple siphash abstraction
      rust: sync: Add dummy LockClassKey implementation for !CONFIG_LOCKDEP
      rust: sync: Replace static LockClassKey refs with a pointer wrapper
      rust: sync: Implement dynamic lockdep class creation
      rust: sync: Classless Lock::new() and pin_init()
      rust: init: Update documentation for new mutex init style
      rust: sync: Add LockdepMap abstraction
      rust: sync: arc: Add lockdep integration
 lib/Kconfig.debug                 |   8 ++
 rust/Makefile                     |   2 +-
 rust/bindings/bindings_helper.h   |   2 +
 rust/helpers.c                    |  24 ++++
 rust/kernel/init.rs               |  22 ++--
 rust/kernel/lib.rs                |   1 +
 rust/kernel/siphash.rs            |  39 +++++++
 rust/kernel/sync.rs               |  33 ++----
 rust/kernel/sync/arc.rs           |  71 +++++++++++-
 rust/kernel/sync/condvar.rs       |   2 +-
 rust/kernel/sync/lock.rs          |  68 ++++++++++-
 rust/kernel/sync/lock/mutex.rs    |  15 ++-
 rust/kernel/sync/lock/spinlock.rs |   2 +-
 rust/kernel/sync/lockdep.rs       | 229 ++++++++++++++++++++++++++++++++++++++
 rust/kernel/sync/no_lockdep.rs    |  38 +++++++
 rust/kernel/types.rs              |   7 +-
 scripts/Makefile.build            |   8 +-
 17 files changed, 525 insertions(+), 46 deletions(-)
---
base-commit: 7eb28ae62e16abc207c90064ad2b824c19566fe2
change-id: 20230714-classless_lockdep-f1d5972fb4ba

Thank you,
~~ Lina
  

Asahi Lina <lina@asahilina.net> writes:
> Begone, lock classes!
> 
> As discussed in meetings/etc, we would really like to support implicit
> lock class creation for Rust code. Right now, lock classes are created
> using macros and passed around (similar to C). Unfortunately, Rust
> macros don't look like Rust functions, which means adding lockdep to a
> type is a breaking API change. This makes Rust mutex creation rather
> ugly, with the new_mutex!() macro and friends.
> 
> Implicit lock classes have to be unique per instantiation code site.
> Notably, with Rust generics and monomorphization, this is not the same
> as unique per generated code instance. If this weren't the case, we
> could use inline functions and asm!() magic to try to create lock
> classes that have the right uniqueness semantics. But that doesn't work,
> since it would create too many lock classes for the same actual lock
> creation in the source code.
> 
> But Rust does have one trick we can use: it can track the caller
> location (as file:line:column), across multiple functions. This works
> using an implicit argument that gets passed around, which is exactly the
> thing we do for lock classes. The tricky bit is that, while the value of
> these Location objects has the semantics we want (unique value per
> source code location), there is no guarantee that they are deduplicated
> in memory.
> 
> So we use a hash table, and map Location values to lock classes. Et
> voila, implicit lock class support!
>
> This lets us clean up the Mutex & co APIs and make them look a lot more
> Rust-like, but it also means we can now throw Lockdep into more APIs
> without breaking the API. And so we can pull a neat trick: adding
> Lockdep support into Arc<T>. This catches cases where the Arc Drop
> implementation could create a locking correctness violation only when
> the reference count drops to 0 at that particular drop site, which is
> otherwise not detectable unless that condition actually happens at
> runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
> to audit, this helps a lot.
> 
> For the initial RFC, this implements the new API only for Mutex. If this
> looks good, I can extend it to CondVar & friends in the next version.
> This series also folds in a few related minor dependencies / changes
> (like the pin_init mutex stuff).

I'm not convinced that this is the right compromise. Moving lockdep
class creation to runtime sounds unfortunate, especially since this
makes them fallible due to memory allocations (I think?).

I would be inclined to keep using macros for this.

Alice
  

On 14/07/2023 19.13, Alice Ryhl wrote:
> Asahi Lina <lina@asahilina.net> writes:
>> Begone, lock classes!
>>
>> As discussed in meetings/etc, we would really like to support implicit
>> lock class creation for Rust code. Right now, lock classes are created
>> using macros and passed around (similar to C). Unfortunately, Rust
>> macros don't look like Rust functions, which means adding lockdep to a
>> type is a breaking API change. This makes Rust mutex creation rather
>> ugly, with the new_mutex!() macro and friends.
>>
>> Implicit lock classes have to be unique per instantiation code site.
>> Notably, with Rust generics and monomorphization, this is not the same
>> as unique per generated code instance. If this weren't the case, we
>> could use inline functions and asm!() magic to try to create lock
>> classes that have the right uniqueness semantics. But that doesn't work,
>> since it would create too many lock classes for the same actual lock
>> creation in the source code.
>>
>> But Rust does have one trick we can use: it can track the caller
>> location (as file:line:column), across multiple functions. This works
>> using an implicit argument that gets passed around, which is exactly the
>> thing we do for lock classes. The tricky bit is that, while the value of
>> these Location objects has the semantics we want (unique value per
>> source code location), there is no guarantee that they are deduplicated
>> in memory.
>>
>> So we use a hash table, and map Location values to lock classes. Et
>> voila, implicit lock class support!
>>
>> This lets us clean up the Mutex & co APIs and make them look a lot more
>> Rust-like, but it also means we can now throw Lockdep into more APIs
>> without breaking the API. And so we can pull a neat trick: adding
>> Lockdep support into Arc<T>. This catches cases where the Arc Drop
>> implementation could create a locking correctness violation only when
>> the reference count drops to 0 at that particular drop site, which is
>> otherwise not detectable unless that condition actually happens at
>> runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
>> to audit, this helps a lot.
>>
>> For the initial RFC, this implements the new API only for Mutex. If this
>> looks good, I can extend it to CondVar & friends in the next version.
>> This series also folds in a few related minor dependencies / changes
>> (like the pin_init mutex stuff).
> 
> I'm not convinced that this is the right compromise. Moving lockdep
> class creation to runtime sounds unfortunate, especially since this
> makes them fallible due to memory allocations (I think?).
> 
> I would be inclined to keep using macros for this.

Most people were very enthusiastic about this change in the meetings... 
it wasn't even my own idea ^^

I don't think the fallibility is an issue. Lockdep is a debugging tool, 
and it doesn't have to handle all possible circumstances perfectly. If 
you are debugging normal lock issues you probably shouldn't be running 
out of RAM, and if you are debugging OOM situations the lock keys would 
normally have been created long before you reach an OOM situation, since 
they would be created the first time a relevant lock class is used. More 
objects of the same class don't cause any more allocations. And the code 
has a fallback for the OOM case, where it just uses the Location object 
as a static lock class. That's not ideal and degrades the quality of the 
lockdep results, but it shouldn't completely break anything.

The advantages of being able to throw lockdep checking into arbitrary 
types, like the Arc<T> thing, are pretty significant. It closes a major 
correctness checking issue we have with Rust and its automagic Drop 
implementations that are almost impossible to properly audit for 
potential locking issues. I think that alone makes this worth it, even 
if you don't use it for normal mutex creation...

~~ Lina
  

Asahi Lina <lina@asahilina.net> writes:
> On 14/07/2023 19.13, Alice Ryhl wrote:
> > Asahi Lina <lina@asahilina.net> writes:
> >> Begone, lock classes!
> >>
> >> As discussed in meetings/etc, we would really like to support implicit
> >> lock class creation for Rust code. Right now, lock classes are created
> >> using macros and passed around (similar to C). Unfortunately, Rust
> >> macros don't look like Rust functions, which means adding lockdep to a
> >> type is a breaking API change. This makes Rust mutex creation rather
> >> ugly, with the new_mutex!() macro and friends.
> >>
> >> Implicit lock classes have to be unique per instantiation code site.
> >> Notably, with Rust generics and monomorphization, this is not the same
> >> as unique per generated code instance. If this weren't the case, we
> >> could use inline functions and asm!() magic to try to create lock
> >> classes that have the right uniqueness semantics. But that doesn't work,
> >> since it would create too many lock classes for the same actual lock
> >> creation in the source code.
> >>
> >> But Rust does have one trick we can use: it can track the caller
> >> location (as file:line:column), across multiple functions. This works
> >> using an implicit argument that gets passed around, which is exactly the
> >> thing we do for lock classes. The tricky bit is that, while the value of
> >> these Location objects has the semantics we want (unique value per
> >> source code location), there is no guarantee that they are deduplicated
> >> in memory.
> >>
> >> So we use a hash table, and map Location values to lock classes. Et
> >> voila, implicit lock class support!
> >>
> >> This lets us clean up the Mutex & co APIs and make them look a lot more
> >> Rust-like, but it also means we can now throw Lockdep into more APIs
> >> without breaking the API. And so we can pull a neat trick: adding
> >> Lockdep support into Arc<T>. This catches cases where the Arc Drop
> >> implementation could create a locking correctness violation only when
> >> the reference count drops to 0 at that particular drop site, which is
> >> otherwise not detectable unless that condition actually happens at
> >> runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
> >> to audit, this helps a lot.
> >>
> >> For the initial RFC, this implements the new API only for Mutex. If this
> >> looks good, I can extend it to CondVar & friends in the next version.
> >> This series also folds in a few related minor dependencies / changes
> >> (like the pin_init mutex stuff).
> > 
> > I'm not convinced that this is the right compromise. Moving lockdep
> > class creation to runtime sounds unfortunate, especially since this
> > makes them fallible due to memory allocations (I think?).
> > 
> > I would be inclined to keep using macros for this.
> 
> Most people were very enthusiastic about this change in the meetings... 
> it wasn't even my own idea ^^

I don't think I was in that meeting. Anyway,
 
> I don't think the fallibility is an issue. Lockdep is a debugging tool, 
> and it doesn't have to handle all possible circumstances perfectly. If 
> you are debugging normal lock issues you probably shouldn't be running 
> out of RAM, and if you are debugging OOM situations the lock keys would 
> normally have been created long before you reach an OOM situation, since 
> they would be created the first time a relevant lock class is used. More 
> objects of the same class don't cause any more allocations. And the code 
> has a fallback for the OOM case, where it just uses the Location object 
> as a static lock class. That's not ideal and degrades the quality of the 
> lockdep results, but it shouldn't completely break anything.

If you have a fallback when the allocation fails, that helps ...

You say that Location objects are not necessarily unique per file
location. In practice, how often are they not unique? Always just using
the Location object as a static lock class seems like it would
significantly simplify this proposal.

> The advantages of being able to throw lockdep checking into arbitrary 
> types, like the Arc<T> thing, are pretty significant. It closes a major 
> correctness checking issue we have with Rust and its automagic Drop 
> implementations that are almost impossible to properly audit for 
> potential locking issues. I think that alone makes this worth it, even 
> if you don't use it for normal mutex creation...

I do agree that there is value in being able to more easily detect
potential deadlocks involving destructors of ref-counted values. I once
had a case of that myself, though lockdep was able to catch it without
this change because it saw the refcount hit zero in the right place.

Alice
  

On Fri, Jul 14, 2023 at 01:59:26PM +0000, Alice Ryhl wrote:
> Asahi Lina <lina@asahilina.net> writes:
> > On 14/07/2023 19.13, Alice Ryhl wrote:
> > > Asahi Lina <lina@asahilina.net> writes:
> > >> Begone, lock classes!
> > >>
> > >> As discussed in meetings/etc, we would really like to support implicit
> > >> lock class creation for Rust code. Right now, lock classes are created

Thanks for looking into this! Could you also copy locking maintainers in
the next version?

> > >> using macros and passed around (similar to C). Unfortunately, Rust
> > >> macros don't look like Rust functions, which means adding lockdep to a
> > >> type is a breaking API change. This makes Rust mutex creation rather
> > >> ugly, with the new_mutex!() macro and friends.
> > >>
> > >> Implicit lock classes have to be unique per instantiation code site.
> > >> Notably, with Rust generics and monomorphization, this is not the same
> > >> as unique per generated code instance. If this weren't the case, we
> > >> could use inline functions and asm!() magic to try to create lock
> > >> classes that have the right uniqueness semantics. But that doesn't work,
> > >> since it would create too many lock classes for the same actual lock
> > >> creation in the source code.
> > >>
> > >> But Rust does have one trick we can use: it can track the caller
> > >> location (as file:line:column), across multiple functions. This works
> > >> using an implicit argument that gets passed around, which is exactly the
> > >> thing we do for lock classes. The tricky bit is that, while the value of
> > >> these Location objects has the semantics we want (unique value per
> > >> source code location), there is no guarantee that they are deduplicated
> > >> in memory.
> > >>
> > >> So we use a hash table, and map Location values to lock classes. Et
> > >> voila, implicit lock class support!
> > >>
> > >> This lets us clean up the Mutex & co APIs and make them look a lot more
> > >> Rust-like, but it also means we can now throw Lockdep into more APIs
> > >> without breaking the API. And so we can pull a neat trick: adding
> > >> Lockdep support into Arc<T>. This catches cases where the Arc Drop
> > >> implementation could create a locking correctness violation only when
> > >> the reference count drops to 0 at that particular drop site, which is
> > >> otherwise not detectable unless that condition actually happens at
> > >> runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
> > >> to audit, this helps a lot.
> > >>
> > >> For the initial RFC, this implements the new API only for Mutex. If this
> > >> looks good, I can extend it to CondVar & friends in the next version.
> > >> This series also folds in a few related minor dependencies / changes
> > >> (like the pin_init mutex stuff).
> > > 
> > > I'm not convinced that this is the right compromise. Moving lockdep
> > > class creation to runtime sounds unfortunate, especially since this
> > > makes them fallible due to memory allocations (I think?).
> > > 
> > > I would be inclined to keep using macros for this.
> > 
> > Most people were very enthusiastic about this change in the meetings... 
> > it wasn't even my own idea ^^
> 
> I don't think I was in that meeting. Anyway,
>  
> > I don't think the fallibility is an issue. Lockdep is a debugging tool, 
> > and it doesn't have to handle all possible circumstances perfectly. If 
> > you are debugging normal lock issues you probably shouldn't be running 
> > out of RAM, and if you are debugging OOM situations the lock keys would 
> > normally have been created long before you reach an OOM situation, since 
> > they would be created the first time a relevant lock class is used. More 
> > objects of the same class don't cause any more allocations. And the code 
> > has a fallback for the OOM case, where it just uses the Location object 
> > as a static lock class. That's not ideal and degrades the quality of the 
> > lockdep results, but it shouldn't completely break anything.
> 
> If you have a fallback when the allocation fails, that helps ...
> 
> You say that Location objects are not necessarily unique per file
> location. In practice, how often are they not unique? Always just using
> the Location object as a static lock class seems like it would
> significantly simplify this proposal.
> 

Agreed. For example, `caller_lock_class_inner` has a Mutex critical
section in it (for the hash table synchronization), that makes it
impossible to be called in preemption disabled contexts, which limits
the usage.

Regards,
Boqun

> > The advantages of being able to throw lockdep checking into arbitrary 
> > types, like the Arc<T> thing, are pretty significant. It closes a major 
> > correctness checking issue we have with Rust and its automagic Drop 
> > implementations that are almost impossible to properly audit for 
> > potential locking issues. I think that alone makes this worth it, even 
> > if you don't use it for normal mutex creation...
> 
> I do agree that there is value in being able to more easily detect
> potential deadlocks involving destructors of ref-counted values. I once
> had a case of that myself, though lockdep was able to catch it without
> this change because it saw the refcount hit zero in the right place.
> 
> Alice
>
  

On Fri, 14 Jul 2023 13:59:26 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> Asahi Lina <lina@asahilina.net> writes:
> > On 14/07/2023 19.13, Alice Ryhl wrote:  
> > > Asahi Lina <lina@asahilina.net> writes:  
> > >> Begone, lock classes!
> > >>
> > >> As discussed in meetings/etc, we would really like to support implicit
> > >> lock class creation for Rust code. Right now, lock classes are created
> > >> using macros and passed around (similar to C). Unfortunately, Rust
> > >> macros don't look like Rust functions, which means adding lockdep to a
> > >> type is a breaking API change. This makes Rust mutex creation rather
> > >> ugly, with the new_mutex!() macro and friends.
> > >>
> > >> Implicit lock classes have to be unique per instantiation code site.
> > >> Notably, with Rust generics and monomorphization, this is not the same
> > >> as unique per generated code instance. If this weren't the case, we
> > >> could use inline functions and asm!() magic to try to create lock
> > >> classes that have the right uniqueness semantics. But that doesn't work,
> > >> since it would create too many lock classes for the same actual lock
> > >> creation in the source code.
> > >>
> > >> But Rust does have one trick we can use: it can track the caller
> > >> location (as file:line:column), across multiple functions. This works
> > >> using an implicit argument that gets passed around, which is exactly the
> > >> thing we do for lock classes. The tricky bit is that, while the value of
> > >> these Location objects has the semantics we want (unique value per
> > >> source code location), there is no guarantee that they are deduplicated
> > >> in memory.
> > >>
> > >> So we use a hash table, and map Location values to lock classes. Et
> > >> voila, implicit lock class support!
> > >>
> > >> This lets us clean up the Mutex & co APIs and make them look a lot more
> > >> Rust-like, but it also means we can now throw Lockdep into more APIs
> > >> without breaking the API. And so we can pull a neat trick: adding
> > >> Lockdep support into Arc<T>. This catches cases where the Arc Drop
> > >> implementation could create a locking correctness violation only when
> > >> the reference count drops to 0 at that particular drop site, which is
> > >> otherwise not detectable unless that condition actually happens at
> > >> runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
> > >> to audit, this helps a lot.
> > >>
> > >> For the initial RFC, this implements the new API only for Mutex. If this
> > >> looks good, I can extend it to CondVar & friends in the next version.
> > >> This series also folds in a few related minor dependencies / changes
> > >> (like the pin_init mutex stuff).  
> > > 
> > > I'm not convinced that this is the right compromise. Moving lockdep
> > > class creation to runtime sounds unfortunate, especially since this
> > > makes them fallible due to memory allocations (I think?).
> > > 
> > > I would be inclined to keep using macros for this.  
> > 
> > Most people were very enthusiastic about this change in the meetings... 
> > it wasn't even my own idea ^^  
> 
> I don't think I was in that meeting. Anyway,

Just for some contexts.

This idea has been discussed multiple times. The earliest discussion
that I can recall is from a tea-break-time discussion in Kangrejos 2022.

It was brought up recently in a discussion related to DRM,
and the consensus was that it's definitely a idea worth exploring.

>  
> > I don't think the fallibility is an issue. Lockdep is a debugging tool, 
> > and it doesn't have to handle all possible circumstances perfectly. If 
> > you are debugging normal lock issues you probably shouldn't be running 
> > out of RAM, and if you are debugging OOM situations the lock keys would 
> > normally have been created long before you reach an OOM situation, since 
> > they would be created the first time a relevant lock class is used. More 
> > objects of the same class don't cause any more allocations. And the code 
> > has a fallback for the OOM case, where it just uses the Location object 
> > as a static lock class. That's not ideal and degrades the quality of the 
> > lockdep results, but it shouldn't completely break anything.  
> 
> If you have a fallback when the allocation fails, that helps ...

I am pretty sure lockdep needs to do some internal allocation anyway
because only address matters for lock class keys. So some extra
allocation probably is fine...

> 
> You say that Location objects are not necessarily unique per file
> location. In practice, how often are they not unique? Always just using
> the Location object as a static lock class seems like it would
> significantly simplify this proposal.

Location objects are constants, so they are not guaranteed to have a
fixed address. With inlining and generics you can very easily get
multiple instances of it. That said, their address is also not
significant, so LLVM is pretty good at merging them back to one single
address, **if everything is linked statically**.

The merging is an optimisation, and is far from guaranteed. With kernel
modules, which effectively is dynamic linking, the address of `Location`
*will* be duplicated if the function invoking a `#[track_caller]`
function is inlined.

An idea was flared when I discussed this with Josh Triplett in last
Kangrejos, that it might be possible to make `Location` generated by
compiler be `static` rather than just normal constants, and then we can
ensure that the address is unique. I tried to prototype this idea but
it didn't seem to work very well because currently you can use
`#[track_caller]` in a const fn but can't refer to statics in a const
fn, so it's a bit hard to desugar. I am pretty sure there are ways
around it, but someone would need to implement it :)

So TL;DR: while in many cases the address is unique, it's far from a
guarantee. It might be possible to guarantee uniqueness but that
requires compiler changes.

> 
> > The advantages of being able to throw lockdep checking into arbitrary 
> > types, like the Arc<T> thing, are pretty significant. It closes a major 
> > correctness checking issue we have with Rust and its automagic Drop 
> > implementations that are almost impossible to properly audit for 
> > potential locking issues. I think that alone makes this worth it, even 
> > if you don't use it for normal mutex creation...  
> 
> I do agree that there is value in being able to more easily detect
> potential deadlocks involving destructors of ref-counted values. I once
> had a case of that myself, though lockdep was able to catch it without
> this change because it saw the refcount hit zero in the right place.
> 
> Alice
>
  

On 15/07/2023 00.21, Boqun Feng wrote:
> On Fri, Jul 14, 2023 at 01:59:26PM +0000, Alice Ryhl wrote:
>> Asahi Lina <lina@asahilina.net> writes:
>>> On 14/07/2023 19.13, Alice Ryhl wrote:
>>>> Asahi Lina <lina@asahilina.net> writes:
>>>>> Begone, lock classes!
>>>>>
>>>>> As discussed in meetings/etc, we would really like to support implicit
>>>>> lock class creation for Rust code. Right now, lock classes are created
> 
> Thanks for looking into this! Could you also copy locking maintainers in
> the next version?

Sure! Sorry, I totally forgot that I needed to do that manually since b4 
doesn't know about rust->C relations...

> 
>>>>> using macros and passed around (similar to C). Unfortunately, Rust
>>>>> macros don't look like Rust functions, which means adding lockdep to a
>>>>> type is a breaking API change. This makes Rust mutex creation rather
>>>>> ugly, with the new_mutex!() macro and friends.
>>>>>
>>>>> Implicit lock classes have to be unique per instantiation code site.
>>>>> Notably, with Rust generics and monomorphization, this is not the same
>>>>> as unique per generated code instance. If this weren't the case, we
>>>>> could use inline functions and asm!() magic to try to create lock
>>>>> classes that have the right uniqueness semantics. But that doesn't work,
>>>>> since it would create too many lock classes for the same actual lock
>>>>> creation in the source code.
>>>>>
>>>>> But Rust does have one trick we can use: it can track the caller
>>>>> location (as file:line:column), across multiple functions. This works
>>>>> using an implicit argument that gets passed around, which is exactly the
>>>>> thing we do for lock classes. The tricky bit is that, while the value of
>>>>> these Location objects has the semantics we want (unique value per
>>>>> source code location), there is no guarantee that they are deduplicated
>>>>> in memory.
>>>>>
>>>>> So we use a hash table, and map Location values to lock classes. Et
>>>>> voila, implicit lock class support!
>>>>>
>>>>> This lets us clean up the Mutex & co APIs and make them look a lot more
>>>>> Rust-like, but it also means we can now throw Lockdep into more APIs
>>>>> without breaking the API. And so we can pull a neat trick: adding
>>>>> Lockdep support into Arc<T>. This catches cases where the Arc Drop
>>>>> implementation could create a locking correctness violation only when
>>>>> the reference count drops to 0 at that particular drop site, which is
>>>>> otherwise not detectable unless that condition actually happens at
>>>>> runtime. Since Drop is "magic" in Rust and Drop codepaths very difficult
>>>>> to audit, this helps a lot.
>>>>>
>>>>> For the initial RFC, this implements the new API only for Mutex. If this
>>>>> looks good, I can extend it to CondVar & friends in the next version.
>>>>> This series also folds in a few related minor dependencies / changes
>>>>> (like the pin_init mutex stuff).
>>>>
>>>> I'm not convinced that this is the right compromise. Moving lockdep
>>>> class creation to runtime sounds unfortunate, especially since this
>>>> makes them fallible due to memory allocations (I think?).
>>>>
>>>> I would be inclined to keep using macros for this.
>>>
>>> Most people were very enthusiastic about this change in the meetings...
>>> it wasn't even my own idea ^^
>>
>> I don't think I was in that meeting. Anyway,
>>   
>>> I don't think the fallibility is an issue. Lockdep is a debugging tool,
>>> and it doesn't have to handle all possible circumstances perfectly. If
>>> you are debugging normal lock issues you probably shouldn't be running
>>> out of RAM, and if you are debugging OOM situations the lock keys would
>>> normally have been created long before you reach an OOM situation, since
>>> they would be created the first time a relevant lock class is used. More
>>> objects of the same class don't cause any more allocations. And the code
>>> has a fallback for the OOM case, where it just uses the Location object
>>> as a static lock class. That's not ideal and degrades the quality of the
>>> lockdep results, but it shouldn't completely break anything.
>>
>> If you have a fallback when the allocation fails, that helps ...
>>
>> You say that Location objects are not necessarily unique per file
>> location. In practice, how often are they not unique? Always just using
>> the Location object as a static lock class seems like it would
>> significantly simplify this proposal.

If a generic type is instantiated from different crates (e.g. kernel 
crate and a driver), it creates separate Location objects. But we also 
have a bigger problem: this breaks module unload, since that leaves lock 
classes dangling. Though that is yet another discussion to have (Rust's 
lifetime semantics kind of break down when you can unload modules!).

>>
> 
> Agreed. For example, `caller_lock_class_inner` has a Mutex critical
> section in it (for the hash table synchronization), that makes it
> impossible to be called in preemption disabled contexts, which limits
> the usage.

Maybe we can just make it a spinlock? The critical section is very short 
for lock classes that already exist (just iterating over the hash 
bucket, which will almost always be length 1), so it's probably more 
efficient to do that than use a mutex anyway. Lockdep itself uses a 
single global spinlock for a bunch of stuff too.

For the new class case it does do an allocation, but I think code 
probably shouldn't be creating locks and things like that with 
preemption disabled / in atomic context? That just seems like a recipe 
for trouble... though this ties into the whole execution context story 
for Rust, which we don't have a terribly good answer for yet, so I think 
it shouldn't block this approach. The macro style lock creation 
primitives still exist for code that really needs the static behavior.

~~ Lina
  

On Sat, Jul 15, 2023 at 03:25:54PM +0100, Gary Guo wrote:
[...]
> > > I don't think the fallibility is an issue. Lockdep is a debugging tool, 
> > > and it doesn't have to handle all possible circumstances perfectly. If 
> > > you are debugging normal lock issues you probably shouldn't be running 
> > > out of RAM, and if you are debugging OOM situations the lock keys would 
> > > normally have been created long before you reach an OOM situation, since 
> > > they would be created the first time a relevant lock class is used. More 
> > > objects of the same class don't cause any more allocations. And the code 
> > > has a fallback for the OOM case, where it just uses the Location object 
> > > as a static lock class. That's not ideal and degrades the quality of the 
> > > lockdep results, but it shouldn't completely break anything.  
> > 
> > If you have a fallback when the allocation fails, that helps ...
> 
> I am pretty sure lockdep needs to do some internal allocation anyway
> because only address matters for lock class keys. So some extra
> allocation probably is fine...
> 

Lockdep uses a few static arrays for its own allocation, but doesn't use
"external" allocatin (i.e. kalloc() and its friends. IIUC, originally
this has to do in this way to avoid recursive calls like:
lockdep->slab->lockdep, but now lockdep has a recursion counter, that's
not a problem any more. However, it's still better that lockdep can work
on its own without relying on other components.

Regards,
Boqun