lkml.org 
[lkml]   [2023]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH RFC] drivers/core: Replace lockdep_set_novalidate_class() with unique class keys
    On Wed, Feb 15, 2023 at 11:45:03AM +0100, Peter Zijlstra wrote:
    > On Tue, Feb 14, 2023 at 08:22:28AM -0800, Boqun Feng wrote:
    >
    > > Ah, right, I was missing the fact that it works with 2 classes...
    > >
    > > But I think with only one class, the nest_lock() still works, right?
    > > In other words, if P and Cn are the same lock class in your example.

    After playing with some self test cases, I found I was wrong again ;-(

    >
    > I don't think so, but I don't think I've carefully considered that case.
    >

    You are right, the same class case will trigger a DEBUG_LOCKS_WARN_ON()
    in the match_held_lock() when releasing the locks.

    > > Also seems I gave a wrong answer to Alan, just to clarify, the following
    > > is not a deadlock to lockdep:
    > >
    > > T1:
    > > mutex_lock(P)
    > > mutex_lock_next_lock(C1, P)
    > > mutex_lock_next_lock(C2, P)
    > > mutex_lock(B)
    > >
    > > T2:
    > > mutex_lock(P)
    > > mutex_lock(B)
    > > mutex_lock_next_lock(C1, P)
    > > mutex_lock_next_lock(C2, P)
    > >
    >
    > This should in fact complain about a CB-BC deadlock, (but I've not
    > tested it, just going on memories of how I implemented it).
    >

    Yes, confirmed by a selftest.

    > > Because of any pair of
    > >
    > > mutex_lock(L);
    > > ... // other locks maybe
    > > mutex_lock_nest_lock(M, L);
    > >
    > > lockdep will not add M into the dependency graph, since it's nested and
    > > should be serialized by L.
    >
    > We do enter M into the dependency graph, but instead ignore M-M
    > recursion. Specifically so that we might catch the above deadlock vs B.

    Right, I mis-read the code, which suggests I should improve it to help
    the future me ;-)

    FWIW, the selftests I used are as follow:

    Regards,
    Boqun

    ------------------------------->8
    diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
    index 8d24279fad05..6aadebad68c1 100644
    --- a/lib/locking-selftest.c
    +++ b/lib/locking-selftest.c
    @@ -60,6 +60,7 @@ __setup("debug_locks_verbose=", setup_debug_locks_verbose);
    #define LOCKTYPE_RTMUTEX 0x20
    #define LOCKTYPE_LL 0x40
    #define LOCKTYPE_SPECIAL 0x80
    +#define LOCKTYPE_NEST 0x100

    static struct ww_acquire_ctx t, t2;
    static struct ww_mutex o, o2, o3;
    @@ -2091,14 +2092,14 @@ static void ww_test_edeadlk_acquire_wrong_slow(void)
    ww_mutex_lock_slow(&o3, &t);
    }

    -static void ww_test_spin_nest_unlocked(void)
    +static void nest_test_spin_nest_unlocked(void)
    {
    spin_lock_nest_lock(&lock_A, &o.base);
    U(A);
    }

    /* This is not a deadlock, because we have X1 to serialize Y1 and Y2 */
    -static void ww_test_spin_nest_lock(void)
    +static void nest_test_spin_nest_lock(void)
    {
    spin_lock(&lock_X1);
    spin_lock_nest_lock(&lock_Y1, &lock_X1);
    @@ -2110,6 +2111,33 @@ static void ww_test_spin_nest_lock(void)
    spin_unlock(&lock_X1);
    }

    +static void nest_test_spin_nest_lock_deadlock(void)
    +{
    + nest_test_spin_nest_lock();
    +
    + /*
    + * Although above is not a deadlokc, but with the following code, Y1 and
    + * A create a ABBA deadlock.
    + */
    + spin_lock(&lock_X1);
    + spin_lock(&lock_A);
    + spin_lock_nest_lock(&lock_Y1, &lock_X1);
    + spin_lock_nest_lock(&lock_Y2, &lock_X1);
    + spin_unlock(&lock_A);
    + spin_unlock(&lock_Y2);
    + spin_unlock(&lock_Y1);
    + spin_unlock(&lock_X1);
    +}
    +
    +/* Not the supported usage */
    +static void nest_test_spin_nest_lock_same_class(void)
    +{
    + spin_lock(&lock_X1);
    + spin_lock_nest_lock(&lock_X2, &lock_X1);
    + spin_unlock(&lock_X2);
    + spin_unlock(&lock_X1);
    +}
    +
    static void ww_test_unneeded_slow(void)
    {
    WWAI(&t);
    @@ -2323,14 +2351,6 @@ static void ww_tests(void)
    dotest(ww_test_edeadlk_acquire_wrong_slow, FAILURE, LOCKTYPE_WW);
    pr_cont("\n");

    - print_testname("spinlock nest unlocked");
    - dotest(ww_test_spin_nest_unlocked, FAILURE, LOCKTYPE_WW);
    - pr_cont("\n");
    -
    - print_testname("spinlock nest test");
    - dotest(ww_test_spin_nest_lock, SUCCESS, LOCKTYPE_WW);
    - pr_cont("\n");
    -
    printk(" -----------------------------------------------------\n");
    printk(" |block | try |context|\n");
    printk(" -----------------------------------------------------\n");
    @@ -2360,6 +2380,27 @@ static void ww_tests(void)
    pr_cont("\n");
    }

    +static void nest_tests(void)
    +{
    + printk(" --------------------------------------------------------------------------\n");
    + printk(" | nest lock tests |\n");
    + printk(" -------------------\n");
    + print_testname("spinlock nest unlocked");
    + dotest(nest_test_spin_nest_unlocked, FAILURE, LOCKTYPE_NEST);
    + pr_cont("\n");
    +
    + print_testname("spinlock nest test");
    + dotest(nest_test_spin_nest_lock, SUCCESS, LOCKTYPE_NEST);
    + pr_cont("\n");
    + print_testname("spinlock nest test dead lock");
    + dotest(nest_test_spin_nest_lock_deadlock, FAILURE, LOCKTYPE_NEST);
    + pr_cont("\n");
    + print_testname("spinlock nest test dead lock");
    + dotest(nest_test_spin_nest_lock_same_class, FAILURE, LOCKTYPE_NEST);
    + pr_cont("\n");
    +
    +}
    +

    /*
    * <in hardirq handler>
    @@ -2966,6 +3007,8 @@ void locking_selftest(void)

    ww_tests();

    + nest_tests();
    +
    force_read_lock_recursive = 0;
    /*
    * queued_read_lock() specific test cases can be put here
    \
     
     \ /
      Last update: 2023-03-27 00:29    [W:3.270 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site