Skip to content

Commit

Permalink
i#4014 dr$sim phys: Avoid malloc in physaddr_t
Browse files Browse the repository at this point in the history
Switches from std::unordered_map in physaddr_t to a drcontainers
hashtable to avoid malloc and make things safe for statically-linked
drmemtrace.

Similarly, switches from std::ostringstream to dr_snprintf in
physaddr_t::init() to avoid malloc.

Tested on a multi-threaded app which hits the post-init malloc warning
without both fixes (test will be added in a forthcoming PR: it cannot
be added now as physaddr_t is not thread-safe yet).

Issue: #4014
  • Loading branch information
derekbruening committed May 27, 2022
1 parent 504765d commit 98da90c
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 21 deletions.
39 changes: 28 additions & 11 deletions clients/drcachesim/tracer/physaddr.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* **********************************************************
* Copyright (c) 2015-2020 Google, Inc. All rights reserved.
* Copyright (c) 2015-2022 Google, Inc. All rights reserved.
* **********************************************************/

/*
Expand Down Expand Up @@ -73,18 +73,29 @@ physaddr_t::physaddr_t()
// No destructor needed either: ifstream closes when destroyed.
}

physaddr_t::~physaddr_t()
{
hashtable_delete(&v2p_);
}

bool
physaddr_t::init()
{
#ifdef LINUX
std::ostringstream oss;
std::string pagemap =
dynamic_cast<std::ostringstream &>(oss << "/proc/" << getpid() << "/pagemap")
.str();
// Some threads may not do much, so start out small.
constexpr int V2P_INITIAL_BITS = 9;
hashtable_init_ex(&v2p_, V2P_INITIAL_BITS, HASH_INTPTR, /*strdup=*/false,
/*synch=*/false, nullptr, nullptr, nullptr);

// We avoid std::ostringstream to avoid malloc use for static linking.
constexpr int MAX_PAGEMAP_FNAME = 64;
char fname[MAX_PAGEMAP_FNAME];
dr_snprintf(fname, sizeof(fname), "/proc/%d/pagemap", getpid());
fname[MAX_PAGEMAP_FNAME - 1] = '\0';
// We can't read pagemap with any buffered i/o, like ifstream, as we'll
// get EINVAL on any non-8-aligned size, and ifstream at least likes to
// read buffers of non-aligned sizes.
fd_ = open(pagemap.c_str(), O_RDONLY);
fd_ = open(fname, O_RDONLY);
// Accessing /proc/pid/pagemap requires privileges on some distributions,
// such as Fedora with recent kernels. We have no choice but to fail there.
return (fd_ != -1);
Expand All @@ -105,7 +116,7 @@ physaddr_t::virtual2physical(addr_t virt)
// Flush the cache and re-sync with the kernel
use_cache = false;
last_vpage_ = PAGE_INVALID;
v2p_.clear();
hashtable_clear(&v2p_);
count_ = 0;
}
if (use_cache) {
Expand All @@ -115,10 +126,14 @@ physaddr_t::virtual2physical(addr_t virt)
return last_ppage_ + PAGE_OFFS(virt);
// XXX i#1703: add (debug-build-only) internal stats here and
// on cache_t::request() fastpath.
std::unordered_map<addr_t, addr_t>::iterator exists = v2p_.find(vpage);
if (exists != v2p_.end()) {
void *lookup = hashtable_lookup(&v2p_, reinterpret_cast<void *>(vpage));
if (lookup != nullptr) {
addr_t ppage = reinterpret_cast<addr_t>(lookup);
// Restore a 0 payload.
if (ppage == 1)
ppage = 0;
last_vpage_ = vpage;
last_ppage_ = exists->second;
last_ppage_ = ppage;
return last_ppage_ + PAGE_OFFS(virt);
}
}
Expand All @@ -142,7 +157,9 @@ physaddr_t::virtual2physical(addr_t virt)
std::cerr << "virtual " << virt << " => physical "
<< (last_ppage_ + PAGE_OFFS(virt)) << std::endl;
}
v2p_[vpage] = last_ppage_;
// Store 0 as 1 since 0 means no entry.
hashtable_add(&v2p_, reinterpret_cast<void *>(vpage),
reinterpret_cast<void *>(last_ppage_ == 0 ? 1 : last_ppage_));
last_vpage_ = vpage;
return last_ppage_ + PAGE_OFFS(virt);
#else
Expand Down
9 changes: 7 additions & 2 deletions clients/drcachesim/tracer/physaddr.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* **********************************************************
* Copyright (c) 2015-2020 Google, Inc. All rights reserved.
* Copyright (c) 2015-2022 Google, Inc. All rights reserved.
* **********************************************************/

/*
Expand Down Expand Up @@ -38,11 +38,14 @@

#include <fstream>
#include <unordered_map>
#include "dr_api.h"
#include "hashtable.h"
#include "../common/trace_entry.h"

class physaddr_t {
public:
physaddr_t();
~physaddr_t();
bool
init();
addr_t
Expand All @@ -54,7 +57,9 @@ class physaddr_t {
addr_t last_vpage_;
addr_t last_ppage_;
int fd_;
std::unordered_map<addr_t, addr_t> v2p_;
// We would use std::unordered_map, but that is not compatible with
// statically linking drmemtrace into an app.
hashtable_t v2p_;
unsigned int count_;
#endif
};
Expand Down
8 changes: 0 additions & 8 deletions clients/drcachesim/tracer/tracer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3116,14 +3116,6 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[])
have_phys = physaddr.init();
if (!have_phys)
NOTIFY(0, "Unable to open pagemap: using virtual addresses.\n");
/* Unfortunately the use of std::unordered_map in physaddr_t calls malloc
* and thus we cannot support it for static linking, so we override the
* DR_DISALLOW_UNSAFE_STATIC declaration.
*/
dr_allow_unsafe_static_behavior();
#ifdef DRMEMTRACE_STATIC
NOTIFY(0, "-use_physical is unsafe with statically linked clients\n");
#endif
}
#ifdef HAS_SNAPPY
if (op_offline.get_value() && snappy_enabled()) {
Expand Down

0 comments on commit 98da90c

Please sign in to comment.