Skip to content

Commit

Permalink
i#6938 sched migrate: Add lock contention stats (#6968)
Browse files Browse the repository at this point in the history
Adds lock contention counters for mutex_dbg_owned::lock() in non-NDEBUG builds.
While it's not there for release builds, the results in non-release should still be
indicative of general lock behavior.

When I just changed the mutex_dbg_owned header, the relese build static drmemtrace
tests (e.g., the burst_* ones) were all failing with floating point exceptions. It
turns out it's due to different compilation units having different values for NDEBUG.
I tried to make them all the same: but we have some files that contain `#undef NDEBUG`
and other complexities. So I split the mutex_dbg_owned implementation into a
.cpp file and have the mutex wrapper extra fields always present.

Prints the stats for sched_lock_ with the other scheduler stats.

Sample results on schedule_stats on a threadsig trace show a lot of
contention even with only 3 cores, which the forthcoming separate runqueue change
will address:

```
$ clients/bin64/drmemtrace_launcher -indir ../build_x64_dbg_tests/drmemtrace.threadsig.5* -core_sharded -cores 3 -tool schedule_stats -verbose 1
[scheduler] Schedule lock acquired     :   2196602
[scheduler] Schedule lock contended    :    257580
```

Issue: #6938
  • Loading branch information
derekbruening authored Sep 9, 2024
1 parent 26e6a1d commit 6630c73
Show file tree
Hide file tree
Showing 5 changed files with 144 additions and 37 deletions.
15 changes: 12 additions & 3 deletions clients/drcachesim/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ add_exported_library(drmemtrace_func_view STATIC tools/func_view.cpp)
add_exported_library(drmemtrace_invariant_checker STATIC tools/invariant_checker.cpp)
add_exported_library(drmemtrace_schedule_stats STATIC tools/schedule_stats.cpp)
add_exported_library(drmemtrace_schedule_file STATIC common/schedule_file.cpp)
add_exported_library(drmemtrace_mutex_dbg_owned STATIC common/mutex_dbg_owned.cpp)

target_link_libraries(drmemtrace_invariant_checker drdecode drmemtrace_schedule_file)

Expand Down Expand Up @@ -284,7 +285,7 @@ target_link_libraries(drmemtrace_launcher drmemtrace_simulator drmemtrace_reuse_
drmemtrace_histogram drmemtrace_reuse_time drmemtrace_basic_counts
drmemtrace_opcode_mix drmemtrace_syscall_mix drmemtrace_view drmemtrace_func_view
drmemtrace_raw2trace directory_iterator drmemtrace_invariant_checker
drmemtrace_schedule_stats drmemtrace_record_filter)
drmemtrace_schedule_stats drmemtrace_record_filter drmemtrace_mutex_dbg_owned)
if (UNIX)
target_link_libraries(drmemtrace_launcher dl)
endif ()
Expand Down Expand Up @@ -331,7 +332,7 @@ add_exported_library(drmemtrace_analyzer STATIC
${snappy_reader}
${lz4_reader}
)
target_link_libraries(drmemtrace_analyzer directory_iterator)
target_link_libraries(drmemtrace_analyzer directory_iterator drmemtrace_mutex_dbg_owned)
if (libsnappy)
target_link_libraries(drmemtrace_analyzer snappy)
endif ()
Expand Down Expand Up @@ -610,6 +611,12 @@ macro(add_win32_flags target)
if (DEBUG AND NOT DEFINED ${target}_uses_configure)
append_property_list(TARGET ${target} COMPILE_DEFINITIONS "DEBUG")
endif ()
if (NOT DEBUG)
get_property(cur TARGET ${target} PROPERTY COMPILE_DEFINITIONS)
if (NOT "${cur}" MATCHES "NDEBUG")
append_property_list(TARGET ${target} COMPILE_DEFINITIONS "NDEBUG")
endif ()
endif ()
if (WIN32)
if (DEBUG)
get_property(cur TARGET ${target} PROPERTY COMPILE_FLAGS)
Expand Down Expand Up @@ -666,6 +673,7 @@ add_win32_flags(drmemtrace_schedule_stats)
add_win32_flags(drmemtrace_schedule_file)
add_win32_flags(directory_iterator)
add_win32_flags(test_helpers)
add_win32_flags(drmemtrace_mutex_dbg_owned)
if (WIN32 AND DEBUG)
get_target_property(sim_srcs drmemtrace_launcher SOURCES)
get_target_property(raw2trace_srcs drraw2trace SOURCES)
Expand Down Expand Up @@ -821,13 +829,13 @@ if (BUILD_TESTS)

add_executable(tool.scheduler.unit_tests tests/scheduler_unit_tests.cpp)
target_link_libraries(tool.scheduler.unit_tests drmemtrace_analyzer test_helpers)
add_win32_flags(tool.scheduler.unit_tests)
if (WIN32)
# We have a dup symbol from linking in DR. Linking libc first doesn't help.
append_property_string(TARGET tool.scheduler.unit_tests LINK_FLAGS
"/force:multiple")
endif ()
configure_DynamoRIO_standalone(tool.scheduler.unit_tests)
add_win32_flags(tool.scheduler.unit_tests)
add_test(NAME tool.scheduler.unit_tests
COMMAND tool.scheduler.unit_tests
${PROJECT_SOURCE_DIR}/clients/drcachesim/tests)
Expand Down Expand Up @@ -881,6 +889,7 @@ if (BUILD_TESTS)
add_executable(tool.drcacheoff.record_filter_unit_tests
tests/record_filter_unit_tests.cpp)
configure_DynamoRIO_standalone(tool.drcacheoff.record_filter_unit_tests)
add_win32_flags(tool.drcacheoff.record_filter_unit_tests)
target_link_libraries(tool.drcacheoff.record_filter_unit_tests drmemtrace_analyzer
drmemtrace_basic_counts drmemtrace_record_filter test_helpers)
add_test(NAME tool.drcacheoff.record_filter_unit_tests
Expand Down
104 changes: 104 additions & 0 deletions clients/drcachesim/common/mutex_dbg_owned.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
/* **********************************************************
* Copyright (c) 2024 Google, Inc. All rights reserved.
* **********************************************************/

/*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* * Redistributions of source code must retain the above copyright notice,
* this list of conditions and the following disclaimer.
*
* * Redistributions in binary form must reproduce the above copyright notice,
* this list of conditions and the following disclaimer in the documentation
* and/or other materials provided with the distribution.
*
* * Neither the name of Google, Inc. nor the names of its contributors may be
* used to endorse or promote products derived from this software without
* specific prior written permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
* AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL VMWARE, INC. OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
* CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
* DAMAGE.
*/

#include <mutex>
#include <thread>
#include "mutex_dbg_owned.h"

namespace dynamorio {
namespace drmemtrace {

void
mutex_dbg_owned::lock()
{
#ifdef NDEBUG
lock_.lock();
#else
bool contended = true;
if (lock_.try_lock())
contended = false;
else
lock_.lock();
owner_ = std::this_thread::get_id();
++count_acquired_;
if (contended)
++count_contended_;
#endif
}

bool
mutex_dbg_owned::try_lock()
{
#ifdef NDEBUG
return lock_.try_lock();
#else
if (lock_.try_lock()) {
owner_ = std::this_thread::get_id();
return true;
}
return false;
#endif
}

void
mutex_dbg_owned::unlock()
{
#ifndef NDEBUG
owner_ = std::thread::id(); // id() creates a no-thread sentinel value.
#endif
lock_.unlock();
}

// This query should only be called when the lock is required to be held
// as it is racy when the lock is not held.
bool
mutex_dbg_owned::owned_by_cur_thread()
{
return owner_ == std::this_thread::get_id();
}

// These statistics only count lock(): they do *not* count try_lock()
// (we could count try_lock with std::atomic on count_contended).
int64_t
mutex_dbg_owned::get_count_acquired()
{
return count_acquired_;
}

int64_t
mutex_dbg_owned::get_count_contended()
{
return count_contended_;
}

} // namespace drmemtrace
} // namespace dynamorio
52 changes: 19 additions & 33 deletions clients/drcachesim/common/mutex_dbg_owned.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
* DAMAGE.
*/

/* mutex_dbg_owned.h: std::mutex plus an assertable owner in debug builds. */
/* mutex_dbg_owned.h: std::mutex plus an assertable owner and stats in debug builds. */

#ifndef _MUTEX_DBG_OWNED_H_
#define _MUTEX_DBG_OWNED_H_ 1
Expand All @@ -43,50 +43,36 @@ namespace drmemtrace {

// A wrapper around std::mutex which adds an owner field for asserts on ownership
// when a lock is required to be held by the caller (where
// std::unique_lock::owns_lock() cannot easily be used). The owner field is only
// maintained when NDEBUG is not defined: i.e., it is targeted for asserts.
// std::unique_lock::owns_lock() cannot easily be used).
// It also adds contention statistics. These additional fields are only maintained
// when NDEBUG is not defined: i.e., they are targeted for asserts and diagnostics.
class mutex_dbg_owned {
public:
void
lock()
{
lock_.lock();
#ifndef NDEBUG
owner_ = std::this_thread::get_id();
#endif
}
lock();
bool
try_lock()
{
#ifdef NDEBUG
return lock_.try_lock();
#else
if (lock_.try_lock()) {
owner_ = std::this_thread::get_id();
return true;
}
return false;
#endif
}
try_lock();
void
unlock()
{
#ifndef NDEBUG
owner_ = std::thread::id(); // id() creates a no-thread sentinel value.
#endif
lock_.unlock();
}
unlock();
// This query should only be called when the lock is required to be held
// as it is racy when the lock is not held.
bool
owned_by_cur_thread()
{
return owner_ == std::this_thread::get_id();
}
owned_by_cur_thread();
// These statistics only count lock(): they do *not* count try_lock()
// (we could count try_lock with std::atomic on count_contended).
int64_t
get_count_acquired();
int64_t
get_count_contended();

private:
std::mutex lock_;
// We do not place these under ifndef NDEBUG as it is too easy to get two
// compilation units with different values for NDEBUG conflicting.
// We thus pay the space cost in NDEBUG build.
std::thread::id owner_;
int64_t count_acquired_ = 0;
int64_t count_contended_ = 0;
};

} // namespace drmemtrace
Expand Down
6 changes: 6 additions & 0 deletions clients/drcachesim/scheduler/scheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,12 @@ scheduler_tmpl_t<RecordType, ReaderType>::~scheduler_tmpl_t()
VPRINT(this, 1, " %-25s: %9" PRId64 "\n", "Migrations",
outputs_[i].stats[memtrace_stream_t::SCHED_STAT_MIGRATIONS]);
}
#ifndef NDEBUG
VPRINT(this, 1, "%-27s: %9" PRId64 "\n", "Schedule lock acquired",
sched_lock_.get_count_acquired());
VPRINT(this, 1, "%-27s: %9" PRId64 "\n", "Schedule lock contended",
sched_lock_.get_count_contended());
#endif
}

template <typename RecordType, typename ReaderType>
Expand Down
4 changes: 3 additions & 1 deletion clients/drcachesim/tests/memref_gen.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* **********************************************************
* Copyright (c) 2021-2023 Google, LLC All rights reserved.
* Copyright (c) 2021-2024 Google, LLC All rights reserved.
* **********************************************************/

/*
Expand Down Expand Up @@ -190,7 +190,9 @@ add_encodings_to_memrefs(instrlist_t *ilist,
{
static const int MAX_DECODE_SIZE = 2048;
byte decode_buf[MAX_DECODE_SIZE];
#ifndef NDEBUG
byte *pc =
#endif
instrlist_encode_to_copy(GLOBAL_DCONTEXT, ilist, decode_buf,
reinterpret_cast<app_pc>(base_addr), nullptr, true);
assert(pc != nullptr);
Expand Down

0 comments on commit 6630c73

Please sign in to comment.