Skip to content

Commit

Permalink
lock: Fix interactions between lock dependency checker and stack checker
Browse files Browse the repository at this point in the history
The lock dependency checker does a few nasty things that can cause
re-entrancy deadlocks in conjunction with the stack checker or
in fact other debug tests.

A lot of it revolves around taking a new lock (dl_lock) as part
of the locking process.

This tries to fix it by making sure we do not hit the stack
checker while holding dl_lock.

We achieve that in part by directly using the low-level __try_lock
and manually unlocking on the dl_lock, and making some functions
"nomcount".

In addition, we mark the dl_lock as being in the console path to
avoid deadlocks with the UART driver.

We move the enabling of the deadlock checker to a separate config
option from DEBUG_LOCKS as well, in case we chose to disable it
by default later on.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Stewart Smith <stewart@linux.ibm.com>
  • Loading branch information
ozbenh authored and stewartsmith committed Aug 16, 2018
1 parent 2925dd0 commit 2f2e0ee
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 15 deletions.
7 changes: 7 additions & 0 deletions core/cpu.c
Original file line number Diff line number Diff line change
Expand Up @@ -714,6 +714,13 @@ struct cpu_thread *find_cpu_by_pir(u32 pir)
return &cpu_stacks[pir].cpu;
}

struct __nomcount cpu_thread *find_cpu_by_pir_nomcount(u32 pir)
{
if (pir > cpu_max_pir)
return NULL;
return &cpu_stacks[pir].cpu;
}

struct cpu_thread *find_cpu_by_server(u32 server_no)
{
struct cpu_thread *t;
Expand Down
45 changes: 30 additions & 15 deletions core/lock.c
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@
bool bust_locks = true;

#ifdef DEBUG_LOCKS
static struct lock dl_lock = LOCK_UNLOCKED;

static void lock_error(struct lock *l, const char *reason, uint16_t err)
static void __nomcount lock_error(struct lock *l, const char *reason, uint16_t err)
{
bust_locks = true;

Expand All @@ -42,13 +41,13 @@ static void lock_error(struct lock *l, const char *reason, uint16_t err)
abort();
}

static void lock_check(struct lock *l)
static inline void __nomcount lock_check(struct lock *l)
{
if ((l->lock_val & 1) && (l->lock_val >> 32) == this_cpu()->pir)
lock_error(l, "Invalid recursive lock", 0);
}

static void unlock_check(struct lock *l)
static inline void __nomcount unlock_check(struct lock *l)
{
if (!(l->lock_val & 1))
lock_error(l, "Unlocking unlocked lock", 1);
Expand Down Expand Up @@ -102,8 +101,21 @@ static inline bool lock_timeout(unsigned long start)
return false;
}
#else
static inline void lock_check(struct lock *l) { };
static inline void unlock_check(struct lock *l) { };
static inline bool lock_timeout(unsigned long s) { return false; }
#endif /* DEBUG_LOCKS */

#if defined(DEADLOCK_CHECKER) && defined(DEBUG_LOCKS)

static struct lock dl_lock = {
.lock_val = 0,
.in_con_path = true,
.owner = LOCK_CALLER
};

/* Find circular dependencies in the lock requests. */
static bool check_deadlock(void)
static __nomcount inline bool check_deadlock(void)
{
uint32_t lock_owner, start, i;
struct cpu_thread *next_cpu;
Expand All @@ -126,7 +138,7 @@ static bool check_deadlock(void)
if (lock_owner == start)
return true;

next_cpu = find_cpu_by_pir(lock_owner);
next_cpu = find_cpu_by_pir_nomcount(lock_owner);

if (!next_cpu)
return false;
Expand All @@ -141,17 +153,20 @@ static bool check_deadlock(void)
static void add_lock_request(struct lock *l)
{
struct cpu_thread *curr = this_cpu();
bool dead;

if (curr->state != cpu_state_active &&
curr->state != cpu_state_os)
return;

/*
* For deadlock detection we must keep the lock states constant
* while doing the deadlock check.
* while doing the deadlock check. However we need to avoid
* clashing with the stack checker, so no mcount and use an
* inline implementation of the lock for the dl_lock
*/
for (;;) {
if (try_lock(&dl_lock))
if (__try_lock(curr, &dl_lock))
break;
smt_lowest();
while (dl_lock.lock_val)
Expand All @@ -161,23 +176,23 @@ static void add_lock_request(struct lock *l)

curr->requested_lock = l;

if (check_deadlock())
lock_error(l, "Deadlock detected", 0);
dead = check_deadlock();

unlock(&dl_lock);
lwsync();
dl_lock.lock_val = 0;

if (dead)
lock_error(l, "Deadlock detected", 0);
}

static void remove_lock_request(void)
{
this_cpu()->requested_lock = NULL;
}
#else
static inline void lock_check(struct lock *l) { };
static inline void unlock_check(struct lock *l) { };
static inline void add_lock_request(struct lock *l) { };
static inline void remove_lock_request(void) { };
static inline bool lock_timeout(unsigned long s) { return false; }
#endif /* DEBUG_LOCKS */
#endif /* #if defined(DEADLOCK_CHECKER) && defined(DEBUG_LOCKS) */

bool lock_held_by_me(struct lock *l)
{
Expand Down
3 changes: 3 additions & 0 deletions include/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@
/* Enable lock debugging */
#define DEBUG_LOCKS 1

/* Enable lock dependency checker */
#define DEADLOCK_CHECKER 1

/* Enable malloc debugging */
#define DEBUG_MALLOC 1

Expand Down
3 changes: 3 additions & 0 deletions include/cpu.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ extern struct cpu_thread *find_cpu_by_node(struct dt_node *cpu);
extern struct cpu_thread *find_cpu_by_server(u32 server_no);
extern struct cpu_thread *find_cpu_by_pir(u32 pir);

/* Used for lock internals to avoid re-entrancy */
extern struct __nomcount cpu_thread *find_cpu_by_pir_nomcount(u32 pir);

extern struct dt_node *get_cpu_node(u32 pir);

/* Iterator */
Expand Down

0 comments on commit 2f2e0ee

Please sign in to comment.