Skip to content

Make mutex lock routines deal with null mutex and general clean up. #4926

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shinrich merged 1 commit into from
Feb 5, 2019
Merged

Make mutex lock routines deal with null mutex and general clean up. #4926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
185 changes: 18 additions & 167 deletions iocore/eventsystem/I_Lock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ Mutex_trylock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif
ProxyMutex *m, EThread *t)
Ptr<ProxyMutex> &m, EThread *t)
{
ink_assert(t != nullptr);
ink_assert(t == (EThread *)this_thread());
Expand Down Expand Up @@ -281,90 +281,12 @@ Mutex_trylock(
return true;
}

inline bool
Mutex_trylock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif
Ptr<ProxyMutex> &m, EThread *t)
{
return Mutex_trylock(
#ifdef DEBUG
location, ahandler,
#endif
m.get(), t);
}

inline bool
Mutex_trylock_spin(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif
ProxyMutex *m, EThread *t, int spincnt = 1)
{
ink_assert(t != nullptr);
if (m->thread_holding != t) {
int locked;
do {
if ((locked = ink_mutex_try_acquire(&m->the_mutex))) {
break;
}
} while (--spincnt);
if (!locked) {
#ifdef DEBUG
lock_waiting(m->srcloc, m->handler);
#ifdef LOCK_CONTENTION_PROFILING
m->unsuccessful_nonblocking_acquires++;
m->nonblocking_acquires++;
m->total_acquires++;
m->print_lock_stats(0);
#endif // LOCK_CONTENTION_PROFILING
#endif // DEBUG
return false;
}
m->thread_holding = t;
ink_assert(m->thread_holding);
#ifdef DEBUG
m->srcloc = location;
m->handler = ahandler;
m->hold_time = Thread::get_hrtime();
#ifdef MAX_LOCK_TAKEN
m->taken++;
#endif // MAX_LOCK_TAKEN
#endif // DEBUG
}
#ifdef DEBUG
#ifdef LOCK_CONTENTION_PROFILING
m->successful_nonblocking_acquires++;
m->nonblocking_acquires++;
m->total_acquires++;
m->print_lock_stats(0);
#endif // LOCK_CONTENTION_PROFILING
#endif // DEBUG
m->nthread_holding++;
return true;
}

inline bool
Mutex_trylock_spin(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif
Ptr<ProxyMutex> &m, EThread *t, int spincnt = 1)
{
return Mutex_trylock_spin(
#ifdef DEBUG
location, ahandler,
#endif
m.get(), t, spincnt);
}

inline int
Mutex_lock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif
ProxyMutex *m, EThread *t)
Ptr<ProxyMutex> &m, EThread *t)
{
ink_assert(t != nullptr);
if (m->thread_holding != t) {
Expand All @@ -391,22 +313,8 @@ Mutex_lock(
return true;
}

inline int
Mutex_lock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif
Ptr<ProxyMutex> &m, EThread *t)
{
return Mutex_lock(
#ifdef DEBUG
location, ahandler,
#endif
m.get(), t);
}

inline void
Mutex_unlock(ProxyMutex *m, EThread *t)
Mutex_unlock(Ptr<ProxyMutex> &m, EThread *t)
{
if (m->nthread_holding) {
ink_assert(t == m->thread_holding);
Expand All @@ -429,12 +337,6 @@ Mutex_unlock(ProxyMutex *m, EThread *t)
}
}

inline void
Mutex_unlock(Ptr<ProxyMutex> &m, EThread *t)
{
Mutex_unlock(m.get(), t);
}

/** Scoped lock class for ProxyMutex
*/
class MutexLock
Expand All @@ -444,20 +346,6 @@ class MutexLock
bool locked_p;

public:
MutexLock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif // DEBUG
ProxyMutex *am, EThread *t)
: m(am), locked_p(true)
{
Mutex_lock(
#ifdef DEBUG
location, ahandler,
#endif // DEBUG
m.get(), t);
}

MutexLock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
Expand All @@ -469,7 +357,7 @@ class MutexLock
#ifdef DEBUG
location, ahandler,
#endif // DEBUG
m.get(), t);
m, t);
}

void
Expand All @@ -493,66 +381,28 @@ class MutexTryLock
bool lock_acquired;

public:
MutexTryLock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif // DEBUG
ProxyMutex *am, EThread *t)
: m(am)
{
lock_acquired = Mutex_trylock(
#ifdef DEBUG
location, ahandler,
#endif // DEBUG
m.get(), t);
}

MutexTryLock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif // DEBUG
Ptr<ProxyMutex> &am, EThread *t)
: m(am)
{
lock_acquired = Mutex_trylock(
#ifdef DEBUG
location, ahandler,
#endif // DEBUG
m.get(), t);
}

MutexTryLock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif // DEBUG
ProxyMutex *am, EThread *t, int sp)
: m(am)
{
lock_acquired = Mutex_trylock_spin(
if (am) {
lock_acquired = Mutex_trylock(
#ifdef DEBUG
location, ahandler,
#endif // DEBUG
m.get(), t, sp);
}

MutexTryLock(
#ifdef DEBUG
const SourceLocation &location, const char *ahandler,
#endif // DEBUG
Ptr<ProxyMutex> &am, EThread *t, int sp)
: m(am)
{
lock_acquired = Mutex_trylock_spin(
#ifdef DEBUG
location, ahandler,
location, ahandler,
#endif // DEBUG
m.get(), t, sp);
m, t);
} else {
lock_acquired = true;
}
}

~MutexTryLock()
{
if (lock_acquired) {
Mutex_unlock(m.get(), m->thread_holding);
if (lock_acquired && m.get()) {
Mutex_unlock(m, m->thread_holding);
}
}

Expand All @@ -561,16 +411,17 @@ class MutexTryLock
void
acquire(EThread *t)
{
MUTEX_TAKE_LOCK(m.get(), t);
lock_acquired = true;
if (m.get()) {
MUTEX_TAKE_LOCK(m, t);
}
}

void
release()
{
ink_assert(lock_acquired); // generate a warning because it shouldn't be done.
if (lock_acquired) {
Mutex_unlock(m.get(), m->thread_holding);
if (lock_acquired && m.get()) {
Mutex_unlock(m, m->thread_holding);
}
lock_acquired = false;
}
Expand Down
37 changes: 20 additions & 17 deletions iocore/hostdb/HostDB.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,13 +449,13 @@ HostDBContinuation::init(HostDBHash const &the_hash, Options const &opt)
void
HostDBContinuation::refresh_hash()
{
ProxyMutex *old_bucket_mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
Ptr<ProxyMutex> old_bucket_mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
// We're not pending DNS anymore.
remove_trigger_pending_dns();
hash.refresh();
// Update the mutex if it's from the bucket.
// Some call sites modify this after calling @c init so need to check.
if (mutex.get() == old_bucket_mutex) {
if (mutex == old_bucket_mutex) {
mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
}
}
Expand Down Expand Up @@ -533,7 +533,7 @@ db_mark_for(IpAddr const &ip)
}

Ptr<HostDBInfo>
probe(ProxyMutex *mutex, HostDBHash const &hash, bool ignore_timeout)
probe(Ptr<ProxyMutex> mutex, HostDBHash const &hash, bool ignore_timeout)
{
// If hostdb is disabled, don't return anything
if (!hostdb_enable) {
Expand Down Expand Up @@ -609,8 +609,8 @@ HostDBProcessor::getby(Continuation *cont, const char *hostname, int len, sockad
HostResStyle host_res_style, int dns_lookup_timeout)
{
HostDBHash hash;
EThread *thread = this_ethread();
ProxyMutex *mutex = thread->mutex.get();
EThread *thread = this_ethread();
Ptr<ProxyMutex> mutex = thread->mutex;
Copy link
Copy Markdown
Member

@SolidWallOfCode SolidWallOfCode Feb 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use auto here, e.g.

auto mutex = thread->mutex;

I will have to defer to @zwoop if that's stylistically preferred.

ip_text_buffer ipb;

HOSTDB_INCREMENT_DYN_STAT(hostdb_total_lookups_stat);
Expand Down Expand Up @@ -640,7 +640,7 @@ HostDBProcessor::getby(Continuation *cont, const char *hostname, int len, sockad
// find the partition lock
//
// TODO: Could we reuse the "mutex" above safely? I think so but not sure.
ProxyMutex *bmutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
Ptr<ProxyMutex> bmutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
MUTEX_TRY_LOCK(lock, bmutex, thread);
MUTEX_TRY_LOCK(lock2, cont->mutex, thread);

Expand Down Expand Up @@ -763,7 +763,7 @@ HostDBProcessor::getSRVbyname_imm(Continuation *cont, process_srv_info_pfn proce
// Attempt to find the result in-line, for level 1 hits
if (!force_dns) {
// find the partition lock
ProxyMutex *bucket_mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
Ptr<ProxyMutex> bucket_mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
MUTEX_TRY_LOCK(lock, bucket_mutex, thread);

// If we can get the lock and a level 1 probe succeeds, return
Expand Down Expand Up @@ -836,7 +836,7 @@ HostDBProcessor::getbyname_imm(Continuation *cont, process_hostdb_info_pfn proce
do {
loop = false; // loop only on explicit set for retry
// find the partition lock
ProxyMutex *bucket_mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
Ptr<ProxyMutex> bucket_mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
SCOPED_MUTEX_LOCK(lock, bucket_mutex, thread);
// do a level 1 probe for immediate result.
Ptr<HostDBInfo> r = probe(bucket_mutex, hash, false);
Expand Down Expand Up @@ -952,8 +952,8 @@ HostDBProcessor::setby(const char *hostname, int len, sockaddr const *ip, HostDB

// Attempt to find the result in-line, for level 1 hits

ProxyMutex *mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
EThread *thread = this_ethread();
Ptr<ProxyMutex> mutex = hostDB.refcountcache->lock_for_key(hash.hash.fold());
EThread *thread = this_ethread();
MUTEX_TRY_LOCK(lock, mutex, thread);

if (lock.is_locked()) {
Expand Down Expand Up @@ -999,7 +999,7 @@ HostDBProcessor::setby_srv(const char *hostname, int len, const char *target, Ho
int
HostDBContinuation::setbyEvent(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
{
Ptr<HostDBInfo> r = probe(mutex.get(), hash, false);
Ptr<HostDBInfo> r = probe(mutex, hash, false);

if (r) {
do_setby(r.get(), &app, hash.host_name, hash.ip, is_srv());
Expand Down Expand Up @@ -1056,8 +1056,11 @@ int
HostDBContinuation::removeEvent(int /* event ATS_UNUSED */, Event *e)
{
Continuation *cont = action.continuation;

MUTEX_TRY_LOCK(lock, cont ? cont->mutex.get() : (ProxyMutex *)nullptr, e->ethread);
Ptr<ProxyMutex> proxy_mutex;
if (cont) {
proxy_mutex = cont->mutex;
}
MUTEX_TRY_LOCK(lock, proxy_mutex, e->ethread);
if (!lock.is_locked()) {
e->schedule_in(HOST_DB_RETRY_PERIOD);
return EVENT_CONT;
Expand All @@ -1068,7 +1071,7 @@ HostDBContinuation::removeEvent(int /* event ATS_UNUSED */, Event *e)
cont->handleEvent(EVENT_HOST_DB_IP_REMOVED, (void *)nullptr);
}
} else {
Ptr<HostDBInfo> r = probe(mutex.get(), hash, false);
Ptr<HostDBInfo> r = probe(mutex, hash, false);
bool res = remove_round_robin(r.get(), hash.host_name, hash.ip);
if (cont) {
cont->handleEvent(EVENT_HOST_DB_IP_REMOVED, res ? static_cast<void *>(&hash.ip) : static_cast<void *>(nullptr));
Expand Down Expand Up @@ -1277,7 +1280,7 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e)
ttl = failed ? 0 : e->ttl / 60;
int ttl_seconds = failed ? 0 : e->ttl; // ebalsa: moving to second accuracy

Ptr<HostDBInfo> old_r = probe(mutex.get(), hash, false);
Ptr<HostDBInfo> old_r = probe(mutex, hash, false);
// If the DNS lookup failed with NXDOMAIN, remove the old record
if (e && e->isNameError() && old_r) {
hostDB.refcountcache->erase(old_r->key);
Expand Down Expand Up @@ -1535,7 +1538,7 @@ HostDBContinuation::iterateEvent(int event, Event *e)
// let's iterate through another record and then reschedule ourself.
if (current_iterate_pos < hostDB.refcountcache->partition_count()) {
// TODO: configurable number at a time?
ProxyMutex *bucket_mutex = hostDB.refcountcache->get_partition(current_iterate_pos).lock.get();
Ptr<ProxyMutex> bucket_mutex = hostDB.refcountcache->get_partition(current_iterate_pos).lock;
MUTEX_TRY_LOCK(lock_bucket, bucket_mutex, t);
if (!lock_bucket.is_locked()) {
// we couldn't get the bucket lock, let's just reschedule and try later.
Expand Down Expand Up @@ -1612,7 +1615,7 @@ HostDBContinuation::probeEvent(int /* event ATS_UNUSED */, Event *e)
if (!force_dns) {
// Do the probe
//
Ptr<HostDBInfo> r = probe(mutex.get(), hash, false);
Ptr<HostDBInfo> r = probe(mutex, hash, false);

if (r) {
HOSTDB_INCREMENT_DYN_STAT(hostdb_total_hits_stat);
Expand Down
Loading