From d80b331f34f95d4aed7f5b75b9fc2f68b581ac70 Mon Sep 17 00:00:00 2001 From: Colin Patrick McCabe Date: Fri, 10 Jun 2011 12:35:25 -0700 Subject: [PATCH] lockdep: code cleanup and de-globalization common/Mutex.cc: this is a duplicate of the code in common/lockdep.cc. Delete the duplicate code. common/lockdep.cc: require the programmer to register a CephContext to use with lockdep. If we don't have one, we don't print anything out. Signed-off-by: Colin McCabe --- src/common/Mutex.cc | 193 ------------------------------------------ src/common/Mutex.h | 10 +-- src/common/lockdep.cc | 39 +++++---- 3 files changed, 22 insertions(+), 220 deletions(-) delete mode 100644 src/common/Mutex.cc diff --git a/src/common/Mutex.cc b/src/common/Mutex.cc deleted file mode 100644 index b7ac971881385..0000000000000 --- a/src/common/Mutex.cc +++ /dev/null @@ -1,193 +0,0 @@ - -#include "common/environment.h" -#include "Mutex.h" - -#ifdef LOCKDEP - -#include "include/types.h" -#include "Clock.h" -#include "BackTrace.h" - -#include - -#include "common/config.h" - -#undef dout -#undef derr -#define dout(l) if (l<=g_conf->debug_lockdep) *_dout << g_clock.now() << " " << std::hex << pthread_self() << std::dec << " lockdep: " -#define derr(l) if (l<=g_conf->debug_lockdep) *_derr << g_clock.now() << " " << std::hex << pthread_self() << std::dec << " lockdep: " - - -pthread_mutex_t lockdep_mutex = PTHREAD_MUTEX_INITIALIZER; - - -#define MAX_LOCKS 100 // increase me as needed - -hash_map lock_ids; -map lock_names; - -int last_id = 0; - -hash_map > held; -BackTrace *follows[MAX_LOCKS][MAX_LOCKS]; // follows[a][b] means b taken after a - - -#define BACKTRACE_SKIP 3 - - - -void Mutex::_register() -{ - pthread_mutex_lock(&lockdep_mutex); - - if (last_id == 0) - for (int i=0; i::iterator p = lock_ids.find(name); - if (p == lock_ids.end()) { - assert(last_id < MAX_LOCKS); - id = last_id++; - lock_ids[name] = id; - lock_names[id] = name; - dout(10) << "registered '" << name << "' as " << id << std::endl; - } else { - id = p->second; - dout(20) << "had '" << name << "' as " << id << std::endl; - } - - pthread_mutex_unlock(&lockdep_mutex); -} - - -// does a follow b? -bool does_follow(int a, int b) -{ - if (follows[a][b]) { - *_dout << std::endl; - dout(0) << "------------------------------------" << std::endl; - dout(0) << "existing dependency " << lock_names[a] << " (" << a << ") -> " << lock_names[b] << " (" << b << ") at: " << std::endl; - follows[a][b]->print(*_dout); - *_dout << std::endl; - return true; - } - - for (int i=0; i " << lock_names[i] << " (" << i << ") at:" << std::endl; - follows[a][i]->print(*_dout); - *_dout << std::endl; - return true; - } - } - - return false; -} - -void Mutex::_will_lock() -{ - pthread_t p = pthread_self(); - if (id < 0) _register(); - - pthread_mutex_lock(&lockdep_mutex); - dout(20) << "_will_lock " << name << " (" << id << ")" << std::endl; - - // check dependency graph - map &m = held[p]; - for (map::iterator p = m.begin(); - p != m.end(); - p++) { - if (p->first == id) { - *_dout << std::endl; - dout(0) << "recursive lock of " << name << " (" << id << ")" << std::endl; - BackTrace *bt = new BackTrace(BACKTRACE_SKIP); - bt->print(*_dout); - if (g_lockdep >= 2) { - *_dout << std::endl; - dout(0) << "previously locked at" << std::endl; - p->second->print(*_dout); - } - *_dout << std::endl; - assert(0); - } - else if (!follows[p->first][id]) { - // new dependency - - // did we just create a cycle? - BackTrace *bt = new BackTrace(BACKTRACE_SKIP); - if (does_follow(id, p->first)) { - dout(0) << "new dependency " << lock_names[p->first] << " (" << p->first << ") -> " << name << " (" << id << ")" - << " creates a cycle at" - << std::endl; - bt->print(*_dout); - *_dout << std::endl; - - dout(0) << "btw, i am holding these locks:" << std::endl; - for (map::iterator q = m.begin(); - q != m.end(); - q++) { - dout(0) << " " << lock_names[q->first] << " (" << q->first << ")" << std::endl; - if (g_lockdep >= 2) { - q->second->print(*_dout); - *_dout << std::endl; - } - } - - *_dout << std::endl; - *_dout << std::endl; - - // don't add this dependency, or we'll get aMutex. cycle in the graph, and - // does_follow() won't terminate. - - assert(0); // actually, we should just die here. - } else { - follows[p->first][id] = bt; - dout(10) << lock_names[p->first] << " -> " << name << " at" << std::endl; - //bt->print(*_dout); - } - } - } - - pthread_mutex_unlock(&lockdep_mutex); -} - -void Mutex::_locked() -{ - pthread_t p = pthread_self(); - - if (id < 0) _register(); - - pthread_mutex_lock(&lockdep_mutex); - dout(20) << "_locked " << name << std::endl; - if (g_lockdep >= 2) - held[p][id] = new BackTrace(BACKTRACE_SKIP); - else - held[p][id] = 0; - pthread_mutex_unlock(&lockdep_mutex); -} - -void Mutex::_unlocked() -{ - pthread_t p = pthread_self(); - - if (id < 0) _register(); - - pthread_mutex_lock(&lockdep_mutex); - dout(20) << "_unlocked " << name << std::endl; - - // don't assert.. lockdep may be enabled at any point in time - //assert(held.count(p)); - //assert(held[p].count(id)); - - delete held[p][id]; - held[p].erase(id); - pthread_mutex_unlock(&lockdep_mutex); -} - - - - - -#endif diff --git a/src/common/Mutex.h b/src/common/Mutex.h index 411b487d1ace6..86a27dc50fcff 100644 --- a/src/common/Mutex.h +++ b/src/common/Mutex.h @@ -15,11 +15,10 @@ #ifndef CEPH_MUTEX_H #define CEPH_MUTEX_H -#include #include "include/assert.h" #include "lockdep.h" -#define LOCKDEP +#include using namespace ceph; @@ -38,7 +37,6 @@ private: void operator=(Mutex &M) {} Mutex( const Mutex &M ) {} -#ifdef LOCKDEP void _register() { id = lockdep_register(name); } @@ -51,12 +49,6 @@ private: void _will_unlock() { // about to unlock id = lockdep_will_unlock(name, id); } -#else - void _register() {} - void _will_lock() {} // about to lock - void _locked() {} // just locked - void _will_unlock() {} // about to unlock -#endif public: Mutex(const char *n, bool r = false, bool ld=true, bool bt=false) : diff --git a/src/common/lockdep.cc b/src/common/lockdep.cc index d3963f91eb142..ceb0a7439f5fc 100644 --- a/src/common/lockdep.cc +++ b/src/common/lockdep.cc @@ -22,6 +22,9 @@ /******* Constants **********/ #define DOUT_SUBSYS lockdep +#undef DOUT_COND +#define DOUT_COND(cct, l) cct && l <= XDOUT_CONDVAR(cct, DOUT_SUBSYS) +#define lockdep_dout(v) ldout(g_lockdep_ceph_ctx, v) #define MAX_LOCKS 100 // increase me as needed #define BACKTRACE_SKIP 3 @@ -33,14 +36,14 @@ struct lockdep_stopper_t { g_lockdep = 0; } }; -static lockdep_stopper_t lockdep_stopper; static pthread_mutex_t lockdep_mutex = PTHREAD_MUTEX_INITIALIZER; +static CephContext *g_lockdep_ceph_ctx = NULL; +static lockdep_stopper_t lockdep_stopper; static hash_map lock_ids; static map lock_names; static int last_id = 0; static hash_map > held; static BackTrace *follows[MAX_LOCKS][MAX_LOCKS]; // follows[a][b] means b taken after a -static CephContext *g_lockdep_ceph_ctx = NULL; /******* Functions **********/ void lockdep_register_ceph_context(CephContext *cct) @@ -57,11 +60,11 @@ int lockdep_dump_locks() for (hash_map >::iterator p = held.begin(); p != held.end(); p++) { - dout(0) << "--- thread " << p->first << " ---" << dendl; + lockdep_dout(0) << "--- thread " << p->first << " ---" << dendl; for (map::iterator q = p->second.begin(); q != p->second.end(); q++) { - dout(0) << " * " << lock_names[q->first] << "\n"; + lockdep_dout(0) << " * " << lock_names[q->first] << "\n"; if (q->second) q->second->print(*_dout); *_dout << dendl; @@ -90,10 +93,10 @@ int lockdep_register(const char *name) id = last_id++; lock_ids[name] = id; lock_names[id] = name; - dout(10) << "registered '" << name << "' as " << id << dendl; + lockdep_dout(10) << "registered '" << name << "' as " << id << dendl; } else { id = p->second; - dout(20) << "had '" << name << "' as " << id << dendl; + lockdep_dout(20) << "had '" << name << "' as " << id << dendl; } pthread_mutex_unlock(&lockdep_mutex); @@ -106,7 +109,7 @@ int lockdep_register(const char *name) static bool does_follow(int a, int b) { if (follows[a][b]) { - dout(0) << "\n"; + lockdep_dout(0) << "\n"; *_dout << "------------------------------------" << "\n"; *_dout << "existing dependency " << lock_names[a] << " (" << a << ") -> " << lock_names[b] << " (" << b << ") at:\n"; @@ -118,7 +121,7 @@ static bool does_follow(int a, int b) for (int i=0; i " << lock_names[i] << " (" << i << ") at:\n"; follows[a][i]->print(*_dout); *_dout << dendl; @@ -135,7 +138,7 @@ int lockdep_will_lock(const char *name, int id) if (id < 0) id = lockdep_register(name); pthread_mutex_lock(&lockdep_mutex); - dout(20) << "_will_lock " << name << " (" << id << ")" << dendl; + lockdep_dout(20) << "_will_lock " << name << " (" << id << ")" << dendl; // check dependency graph map &m = held[p]; @@ -143,7 +146,7 @@ int lockdep_will_lock(const char *name, int id) p != m.end(); p++) { if (p->first == id) { - dout(0) << "\n"; + lockdep_dout(0) << "\n"; *_dout << "recursive lock of " << name << " (" << id << ")\n"; BackTrace *bt = new BackTrace(BACKTRACE_SKIP); bt->print(*_dout); @@ -160,25 +163,25 @@ int lockdep_will_lock(const char *name, int id) // did we just create a cycle? BackTrace *bt = new BackTrace(BACKTRACE_SKIP); if (does_follow(id, p->first)) { - dout(0) << "new dependency " << lock_names[p->first] + lockdep_dout(0) << "new dependency " << lock_names[p->first] << " (" << p->first << ") -> " << name << " (" << id << ")" << " creates a cycle at\n"; bt->print(*_dout); *_dout << dendl; - dout(0) << "btw, i am holding these locks:" << dendl; + lockdep_dout(0) << "btw, i am holding these locks:" << dendl; for (map::iterator q = m.begin(); q != m.end(); q++) { - dout(0) << " " << lock_names[q->first] << " (" << q->first << ")" << dendl; + lockdep_dout(0) << " " << lock_names[q->first] << " (" << q->first << ")" << dendl; if (q->second) { - dout(0) << " "; + lockdep_dout(0) << " "; q->second->print(*_dout); *_dout << dendl; } } - dout(0) << "\n" << dendl; + lockdep_dout(0) << "\n" << dendl; // don't add this dependency, or we'll get aMutex. cycle in the graph, and // does_follow() won't terminate. @@ -186,7 +189,7 @@ int lockdep_will_lock(const char *name, int id) assert(0); // actually, we should just die here. } else { follows[p->first][id] = bt; - dout(10) << lock_names[p->first] << " -> " << name << " at" << dendl; + lockdep_dout(10) << lock_names[p->first] << " -> " << name << " at" << dendl; //bt->print(*_dout); } } @@ -203,7 +206,7 @@ int lockdep_locked(const char *name, int id, bool force_backtrace) if (id < 0) id = lockdep_register(name); pthread_mutex_lock(&lockdep_mutex); - dout(20) << "_locked " << name << dendl; + lockdep_dout(20) << "_locked " << name << dendl; if (g_lockdep >= 2 || force_backtrace) held[p][id] = new BackTrace(BACKTRACE_SKIP); else @@ -223,7 +226,7 @@ int lockdep_will_unlock(const char *name, int id) } pthread_mutex_lock(&lockdep_mutex); - dout(20) << "_will_unlock " << name << dendl; + lockdep_dout(20) << "_will_unlock " << name << dendl; // don't assert.. lockdep may be enabled at any point in time //assert(held.count(p)); -- 2.39.5