Skip to content

src: retrieve .text coordinates for large pages #33116

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

Closed
gabrielschulhof wants to merge 3 commits into from
Closed

src: retrieve .text coordinates for large pages #33116

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31f32ba
src: retrieve .text coordinates for large pages
Apr 28, 2020
1979429
address review comments
Apr 28, 2020
2525cb9
heed the linter
Apr 28, 2020
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
25 changes: 0 additions & 25 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,6 @@
'node_mksnapshot_exec': '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)node_mksnapshot<(EXECUTABLE_SUFFIX)',
'mkcodecache_exec': '<(PRODUCT_DIR)/<(EXECUTABLE_PREFIX)mkcodecache<(EXECUTABLE_SUFFIX)',
'conditions': [
['GENERATOR == "ninja"', {
'node_text_start_object_path': 'src/large_pages/node_text_start.node_text_start.o'
}, {
'node_text_start_object_path': 'node_text_start/src/large_pages/node_text_start.o'
}],
[ 'node_shared=="true"', {
'node_target_type%': 'shared_library',
'conditions': [
Expand Down Expand Up @@ -320,19 +315,6 @@
},

'targets': [
{
'target_name': 'node_text_start',
'type': 'none',
'conditions': [
[ 'OS in "linux freebsd" and '
'target_arch=="x64"', {
'type': 'static_library',
'sources': [
'src/large_pages/node_text_start.S'
]
}],
]
},
{
'target_name': '<(node_core_target_name)',
'type': 'executable',
Expand Down Expand Up @@ -513,13 +495,6 @@
'src/node_snapshot_stub.cc'
],
}],
[ 'OS in "linux freebsd" and '
'target_arch=="x64"', {
'dependencies': [ 'node_text_start' ],
'ldflags+': [
'<(obj_dir)/<(node_text_start_object_path)'
]
}],
],
}, # node_core_target_name
{
Expand Down
238 changes: 131 additions & 107 deletions src/large_pages/node_large_page.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,11 @@
//
// 1: Find the Node.js binary's `.text` section in memory. This is done below in
// `FindNodeTextRegion`. It is accomplished in a platform-specific way. On
// Linux and FreeBSD, `dl_iterate_phdr(3)` is used. When the region is found,
// it is "trimmed" as follows:
// * Modify the start to point to the very beginning of the Node.js `.text`
// section (from symbol `__node_text_start` declared in node_text_start.S).
// * Possibly modify the end to account for the `lpstub` section which
// contains `MoveTextRegionToLargePages`, the function we do not wish to
// move (see below).
// Linux and FreeBSD, `dl_iterate_phdr(3)` is used to find the in-memory base
// address of the executable, which is then opened and the ELF section header
// for the `.text` section is retrieved in order to find the offset of the
// `.text` section wrt. the base address and its size. When the region is
// found, it is trimmed as follows:
// * Align the address of the start to its nearest higher large page
// boundary.
// * Align the address of the end to its nearest lower large page boundary.
Expand Down Expand Up @@ -64,6 +62,7 @@
#include "debug_utils-inl.h"

#if defined(__linux__) || defined(__FreeBSD__)
#include <unistd.h> // close
#if defined(__linux__)
#ifndef _GNU_SOURCE
#define _GNU_SOURCE
Expand All @@ -89,16 +88,6 @@
#include <string>
#include <fstream>

#if defined(__linux__) || defined(__FreeBSD__)
extern "C" {
// This symbol must be declared weak because this file becomes part of all
// Node.js targets (like node_mksnapshot, node_mkcodecache, and cctest) and
// those files do not supply the symbol.
extern char __attribute__((weak)) __node_text_start;
extern char __start_lpstub;
} // extern "C"
#endif // defined(__linux__) || defined(__FreeBSD__)

#endif // defined(NODE_ENABLE_LARGE_CODE_PAGES) && NODE_ENABLE_LARGE_CODE_PAGES
namespace node {
#if defined(NODE_ENABLE_LARGE_CODE_PAGES) && NODE_ENABLE_LARGE_CODE_PAGES
Expand All @@ -108,8 +97,7 @@ namespace {
struct text_region {
char* from;
char* to;
int total_hugepages;
bool found_text_region;
bool found_text_region;
};

static const size_t hps = 2L * 1024 * 1024;
Expand Down Expand Up @@ -144,43 +132,120 @@ inline uintptr_t hugepage_align_down(uintptr_t addr) {
#endif // ifndef ElfW
#endif // defined(__FreeBSD__)

#define CONSTRUCT_ONLY(class_name) \
class_name(const class_name&) = delete; \
class_name(class_name&&) = delete; \
void operator= (const class_name&) = delete; \
void operator= (const class_name&&) = delete
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Apr 28, 2020

Choose a reason for hiding this comment

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

We used to have a DISALLOW_COPY_AND_ASSIGN macro for this but we moved to just writing them out by hand, see #26634. Can you do that here too?


// Functions in this class must always be inlined because they must end up in
// the `lpstub` section rather than the `.text` section.
class MemoryMapPointer {
public:
CONSTRUCT_ONLY(MemoryMapPointer);
FORCE_INLINE explicit MemoryMapPointer() {}
FORCE_INLINE bool operator==(void* rhs) const { return mem_ == rhs; }
FORCE_INLINE void* mem() const { return mem_; }
FORCE_INLINE void Reset(void* start,
size_t size,
int prot,
int flags,
int fd = -1,
size_t offset = 0) {
Reset(mmap(start, size, prot, flags, fd, offset), size);
}
FORCE_INLINE void Reset() { Reset(nullptr, 0); }
FORCE_INLINE ~MemoryMapPointer() {
if (mem_ == nullptr) return;
if (mem_ == MAP_FAILED) return;
if (munmap(mem_, size_) == 0) return;
PrintSystemError(errno);
}

protected:
FORCE_INLINE void Reset(void* data, size_t size) {
mem_ = data;
size_ = size;
}

private:
size_t size_ = 0;
void* mem_ = nullptr;
};

class MappedFilePointer: public MemoryMapPointer {
public:
CONSTRUCT_ONLY(MappedFilePointer);
FORCE_INLINE explicit MappedFilePointer() {}
FORCE_INLINE void Reset(void* start,
const char* fname,
int prot,
int flags,
size_t offset = 0) {
struct stat file_info;
if (stat(fname, &file_info) == -1) goto fail;

fd_ = open(fname, O_RDONLY);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Apr 28, 2020

Choose a reason for hiding this comment

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

open(2) is interruptible by a signal. You should retry on EINTR:

Suggested change
fd_ = open(fname, O_RDONLY);
do
fd_ = open(fname, O_RDONLY);
while (fd == -1 && errno == EINTR);

As well, stat + open is race-y. Not that it really matters here but you still should do open + fstat. :-)

if (fd_ == -1) goto fail;

MemoryMapPointer::Reset(start, file_info.st_size, prot, flags, fd_, offset);
return;
fail:
MemoryMapPointer::Reset(MAP_FAILED, 0);
}

FORCE_INLINE ~MappedFilePointer() {
if (fd_ == -1) return;
if (close(fd_) == 0) return;
PrintSystemError(errno);
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Apr 28, 2020

Choose a reason for hiding this comment

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

Can you if (errno == EINTR || errno == EINPROGRESS) return;?

close(2) is interruptible but you shouldn't retry - it's going to get closed anyway - and printing an error isn't useful.

Copy link
Copy Markdown
Contributor Author

@gabrielschulhof gabrielschulhof Apr 28, 2020

Choose a reason for hiding this comment

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

I added this.

}

private:
int fd_ = -1;
};

struct dl_iterate_params {
uintptr_t start;
uintptr_t end;
uintptr_t reference_sym;
uintptr_t base_address;
std::string exename;
};

int FindMapping(struct dl_phdr_info* info, size_t, void* data) {
int FindBaseAddress(struct dl_phdr_info* info, size_t, void* data) {
auto dl_params = static_cast<dl_iterate_params*>(data);
if (dl_params->exename == std::string(info->dlpi_name)) {
for (int idx = 0; idx < info->dlpi_phnum; idx++) {
const ElfW(Phdr)* phdr = &info->dlpi_phdr[idx];
if (phdr->p_type == PT_LOAD && (phdr->p_flags & PF_X)) {
uintptr_t start = info->dlpi_addr + phdr->p_vaddr;
uintptr_t end = start + phdr->p_memsz;

if (dl_params->reference_sym >= start &&
dl_params->reference_sym <= end) {
dl_params->start = start;
dl_params->end = end;
return 1;
}
}
}
dl_params->base_address = info->dlpi_addr;
return 1;
}
return 0;
}
#endif // defined(__linux__) || defined(__FreeBSD__)

bool FindTextSection(const std::string& exename, ElfW(Shdr)* text_section) {
MappedFilePointer exe;
exe.Reset(nullptr, exename.c_str(), PROT_READ, MAP_PRIVATE);
if (exe.mem() == MAP_FAILED) return false;

const char* exe_start = static_cast<char*>(exe.mem());
const ElfW(Ehdr)* ehdr = reinterpret_cast<const ElfW(Ehdr)*>(exe_start);
const ElfW(Shdr)* shdrs =
reinterpret_cast<const ElfW(Shdr)*>(exe_start + ehdr->e_shoff);
const char* snames = exe_start + shdrs[ehdr->e_shstrndx].sh_offset;

for (uint32_t idx = 0; idx < ehdr->e_shnum; idx++) {
const ElfW(Shdr)* sh = &shdrs[idx];
const char* section_name = static_cast<const char*>(&snames[sh->sh_name]);
if (std::string(section_name) == ".text") {
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Apr 28, 2020

Choose a reason for hiding this comment

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

Suggested change
if (std::string(section_name) == ".text") {
if (0 == strcmp(section_name, ".text")) {

It's kind of wasteful to create a heap-allocated throwaway string. It's probably going to be stored inline for short strings but you get my point.

*text_section = *sh;
return true;
}
}

return false;
}

struct text_region FindNodeTextRegion() {
struct text_region nregion;
nregion.found_text_region = false;
struct text_region nregion = { nullptr, nullptr, false };
#if defined(__linux__) || defined(__FreeBSD__)
dl_iterate_params dl_params = {
0, 0, reinterpret_cast<uintptr_t>(&__node_text_start), ""
};
uintptr_t lpstub_start = reinterpret_cast<uintptr_t>(&__start_lpstub);
dl_iterate_params dl_params = { 0, "" };

#if defined(__FreeBSD__)
// On FreeBSD we need the name of the binary, because `dl_iterate_phdr` does
Expand All @@ -195,33 +260,28 @@ struct text_region FindNodeTextRegion() {
}
#endif // defined(__FreeBSD__)

if (dl_iterate_phdr(FindMapping, &dl_params) == 1) {
Debug("Hugepages info: start: %p - sym: %p - end: %p\n",
reinterpret_cast<void*>(dl_params.start),
reinterpret_cast<void*>(dl_params.reference_sym),
reinterpret_cast<void*>(dl_params.end));

dl_params.start = dl_params.reference_sym;
if (lpstub_start > dl_params.start && lpstub_start <= dl_params.end) {
Debug("Hugepages info: Trimming end for lpstub: %p\n",
reinterpret_cast<void*>(lpstub_start));
dl_params.end = lpstub_start;
}
if (dl_iterate_phdr(FindBaseAddress, &dl_params) == 0) return nregion;

if (dl_params.start < dl_params.end) {
char* from = reinterpret_cast<char*>(hugepage_align_up(dl_params.start));
char* to = reinterpret_cast<char*>(hugepage_align_down(dl_params.end));
Debug("Hugepages info: Aligned range is %p - %p\n", from, to);
if (from < to) {
size_t pagecount = (to - from) / hps;
if (pagecount > 0) {
nregion.found_text_region = true;
nregion.from = from;
nregion.to = to;
nregion.total_hugepages = pagecount;
}
}
}
ElfW(Shdr) text_section;
#if defined(__linux__)
dl_params.exename = "/proc/self/exe";
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Apr 28, 2020

Choose a reason for hiding this comment

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

This assumes /proc is mounted but that need not be the case.

#endif // define(__linux__)
if (!FindTextSection(dl_params.exename, &text_section)) return nregion;

uintptr_t start = dl_params.base_address + text_section.sh_addr;
uintptr_t end = start + text_section.sh_size;
Debug("Hugepages info: start: %p - end: %p\n",
reinterpret_cast<void*>(start),
reinterpret_cast<void*>(end));

char* from = reinterpret_cast<char*>(hugepage_align_up(start));
char* to = reinterpret_cast<char*>(hugepage_align_down(end));
Debug("Hugepages info: Aligned range is %p - %p\n", from, to);

if (from < to && ((to - from) / hps) > 0) {
nregion.from = from;
nregion.to = to;
nregion.found_text_region = true;
}
#elif defined(__APPLE__)
struct vm_region_submap_info_64 map;
Expand Down Expand Up @@ -249,7 +309,6 @@ struct text_region FindNodeTextRegion() {
nregion.found_text_region = true;
nregion.from = start;
nregion.to = end;
nregion.total_hugepages = esize / hps;
break;
}

Expand All @@ -258,7 +317,8 @@ struct text_region FindNodeTextRegion() {
}
}
#endif
Debug("Hugepages info: Found %d huge pages\n", nregion.total_hugepages);
Debug("Hugepages info: Found %d huge pages\n",
(nregion.to - nregion.from) / hps);
return nregion;
}

Expand Down Expand Up @@ -294,42 +354,6 @@ bool IsSuperPagesEnabled() {
}
#endif

// Functions in this class must always be inlined because they must end up in
// the `lpstub` section rather than the `.text` section.
class MemoryMapPointer {
public:
FORCE_INLINE explicit MemoryMapPointer() {}
FORCE_INLINE bool operator==(void* rhs) const { return mem_ == rhs; }
FORCE_INLINE void* mem() const { return mem_; }
MemoryMapPointer(const MemoryMapPointer&) = delete;
MemoryMapPointer(MemoryMapPointer&&) = delete;
void operator= (const MemoryMapPointer&) = delete;
void operator= (const MemoryMapPointer&&) = delete;
FORCE_INLINE void Reset(void* start,
size_t size,
int prot,
int flags,
int fd = -1,
size_t offset = 0) {
mem_ = mmap(start, size, prot, flags, fd, offset);
size_ = size;
}
FORCE_INLINE void Reset() {
mem_ = nullptr;
size_ = 0;
}
FORCE_INLINE ~MemoryMapPointer() {
if (mem_ == nullptr) return;
if (mem_ == MAP_FAILED) return;
if (munmap(mem_, size_) == 0) return;
PrintSystemError(errno);
}

private:
size_t size_ = 0;
void* mem_ = nullptr;
};

} // End of anonymous namespace

int
Expand Down
5 changes: 0 additions & 5 deletions src/large_pages/node_text_start.S
View file
Open in desktop

This file was deleted.