assert_mtx: treat LA_LOCKED as the same of LA_XLOCKED.

[delphij]

Summary

assert_mtx: treat LA_LOCKED as the same of LA_XLOCKED.

Test Plan

Test Intel DRM driver with INVARIANTS and WITNESS enabled.

[• hselasky]

Why not extend the switch case in __mtx_assert ??

void
__mtx_assert(const volatile uintptr_t *c, int what, const char *file, int line)
{
        const struct mtx *m;

        if (panicstr != NULL || dumping || SCHEDULER_STOPPED())
                return;

        m = mtxlock2mtx(c);

        switch (what) {
        case LA_LOCKED:
        case LA_LOCKED | MA_RECURSED:
        case LA_LOCKED | MA_NOTRECURSED:     
        case MA_OWNED:
        case MA_OWNED | MA_RECURSED:
        case MA_OWNED | MA_NOTRECURSED:

[mjg]

I disagree with mixing up LA_ and MA_ namespaces.

I don't have a strong opinion about the patch to modify LA_LOCKED into LA_XLOCKED. What's the rationale behind it? Is there a problem passing a more appropriate flag in DRM code?

[jhb]

I would just use hselasky's approach of extending the case statements, but this is also ok.

[jhb]

In D21362#464773, @mjg wrote:

I disagree with mixing up LA_ and MA_ namespaces.

This is to avoid having lookup tables in every single lc_assert method.

I don't have a strong opinion about the patch to modify LA_LOCKED into LA_XLOCKED. What's the rationale behind it? Is there a problem passing a more appropriate flag in DRM code?

The linux lockdep API assumes LA_LOCKED semantics (meaning either a shared lock or write lock is ok). lc_assert isn't widely used in the tree, so this just hasn't been hit before.

[cem]

In D21362#464855, @jhb wrote:

In D21362#464773, @mjg wrote:

I disagree with mixing up LA_ and MA_ namespaces.

This is to avoid having lookup tables in every single lc_assert method.

Thanks, I was also curious about the motivation / context for the change.

The linux lockdep API assumes LA_LOCKED semantics (meaning either a shared lock or write lock is ok). lc_assert isn't widely used in the tree, so this just hasn't been hit before.

Would it make more sense to add a LC_SHARED to lock_class::lc_flags and define a short wrapper in sys/lock.h?

static inline void
lock_assert(const struct lock_object *lo, int la_what)
{
    if ((lo->lc_flags & LC_SHARED) == 0 && (la_what & LA_LOCKED) != 0) {
        la_what &= ~LA_LOCKED;
        la_what |= LA_XLOCKED;
    }
    lo->lc_assert(lo, la_what);
}

sys/kern/kern_mutex.c

175

This is redundant with the declaration (that's ok, just noting it).

[mjg]

All other routines are read-write and I presume should translate automatically. Looks like everything is just redefined in terms LA_ macros anyway. If they are to mix, what's the point of having all the redefinitions?

So is the problem that the code takes exclusive-only spinlocks and asserts them with mere lockdep_assert_held without the mode? This can be patched in the macro by checking the lock type, but perhaps it's a waste.

I think it is slightly iffy to mix this in as in the patch but it's fine enough with a comment explaining why it was done. Alternatively perhaps MA_OWNED can instead be redefined as LA_LOCKED and chances are this will automatically take care of everything.

[jhb]

In D21362#464856, @cem wrote:

In D21362#464855, @jhb wrote:

In D21362#464773, @mjg wrote:

I disagree with mixing up LA_ and MA_ namespaces.

This is to avoid having lookup tables in every single lc_assert method.

Thanks, I was also curious about the motivation / context for the change.

The linux lockdep API assumes LA_LOCKED semantics (meaning either a shared lock or write lock is ok). lc_assert isn't widely used in the tree, so this just hasn't been hit before.

Would it make more sense to add a LC_SHARED to lock_class::lc_flags and define a short wrapper in sys/lock.h?

static inline void
lock_assert(const struct lock_object *lo, int la_what)
{
    if ((lo->lc_flags & LC_SHARED) == 0 && (la_what & LA_LOCKED) != 0) {
        la_what &= ~LA_LOCKED;
        la_what |= LA_XLOCKED;
    }
    lo->lc_assert(lo, la_what);
}

I think it's simplest to just patch mtx as it is the only lock class affected, and we're not likely to add more any time soon.

[jhb]

In D21362#464861, @mjg wrote:

All other routines are read-write and I presume should translate automatically. Looks like everything is just redefined in terms LA_ macros anyway. If they are to mix, what's the point of having all the redefinitions?

Originally we only had mtx_assert() and sx_assert() with their own sets of constants. As part of adding sx support to witness, I added a witness_assert() so that sx could have stronger assertions when witness was enabled (specifically SA_SLOCKED would be fully reliably), and for that I added LA_* for witness to use and updated mtx and sx constants to be based on those. Other locks simply followed the same model. This is why other locks than mutex have constants that mostly match the LA_* names as well (LA_LOCKED vs MA_OWNED). Renaming the mutex constants and or converting everyone to just use LA_* directly would seem a bit gratuitous.

So is the problem that the code takes exclusive-only spinlocks and asserts them with mere lockdep_assert_held without the mode? This can be patched in the macro by checking the lock type, but perhaps it's a waste.

You can't easily check the lock type in the macro in C, that's why the macro uses lc_assert. However, given the intention of lc_assert and the meaning of LA_LOCKED, it's really just a bug (IMO) that assert_mtx doesn't honor LA_LOCKED and I think fixing that is the simplest approach.

I think it is slightly iffy to mix this in as in the patch but it's fine enough with a comment explaining why it was done. Alternatively perhaps MA_OWNED can instead be redefined as LA_LOCKED and chances are this will automatically take care of everything.

No, the timeout code (the one other use of lc_assert() uses LA_XLOCKED, so mtx needs to handle both LA_LOCKED and LA_XLOCKED as places need both to work now.

[mjg]

In D21362#464874, @jhb wrote:

In D21362#464861, @mjg wrote:

All other routines are read-write and I presume should translate automatically. Looks like everything is just redefined in terms LA_ macros anyway. If they are to mix, what's the point of having all the redefinitions?

Originally we only had mtx_assert() and sx_assert() with their own sets of constants. As part of adding sx support to witness, I added a witness_assert() so that sx could have stronger assertions when witness was enabled (specifically SA_SLOCKED would be fully reliably), and for that I added LA_* for witness to use and updated mtx and sx constants to be based on those. Other locks simply followed the same model. This is why other locks than mutex have constants that mostly match the LA_* names as well (LA_LOCKED vs MA_OWNED). Renaming the mutex constants and or converting everyone to just use LA_* directly would seem a bit gratuitous.

I meant if macros are there the separation should be maintained, sudden LA_ macros look out of place.

So is the problem that the code takes exclusive-only spinlocks and asserts them with mere lockdep_assert_held without the mode? This can be patched in the macro by checking the lock type, but perhaps it's a waste.

You can't easily check the lock type in the macro in C, that's why the macro uses lc_assert. However, given the intention of lc_assert and the meaning of LA_LOCKED, it's really just a bug (IMO) that assert_mtx doesn't honor LA_LOCKED and I think fixing that is the simplest approach.

AFAIR it is hackable with sufficiently bad preprocessor magic.

I think it is slightly iffy to mix this in as in the patch but it's fine enough with a comment explaining why it was done. Alternatively perhaps MA_OWNED can instead be redefined as LA_LOCKED and chances are this will automatically take care of everything.

No, the timeout code (the one other use of lc_assert() uses LA_XLOCKED, so mtx needs to handle both LA_LOCKED and LA_XLOCKED as places need both to work now.

Ok.

I think this received enough bikeshedding. AFAIU everyone agrees the proposed patch is fine enough (but I would argue it wants a comment explaining the change).

[cem]

I'd like the commit message to provide more context but the change is good.

In D21362#464873, @jhb wrote:

I think it's simplest to just patch mtx as it is the only lock class affected, and we're not likely to add more any time soon.

Sure, that makes sense.

In D21362#464941, @mjg wrote:

I meant if macros are there the separation should be maintained, sudden LA_ macros look out of place.

The lc_*() APIs operate on LA_ macros; it's the job of assert_mtx, etc, to translate those into MA_ macro space. Given they're the same numerical value, on purpose... yeah, it's pretty silly. I would prefer we just got rid of the separate named constants (eventually).

[jhb]

I think add a comment as @mjg requests and the existing patch is good to go. Thanks Xin Li!