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

Jump to bottom

Closed

gabrielschulhof wants to merge 3 commits into nodejs:master from gabrielschulhof:large-pages-read-elf

Contributor

gabrielschulhof commented Apr 28, 2020

After locating the in-memory base address of the Node.js executable
using dl_iterate_phdr(3) we open the on-disk executable file and
extract the address and size of the .text section. We then add the
in-memory base address to the offset to locate the .text section in
memory.

This approach removes the need for a symbol to tell us where the
.text section is located, and, unlike the previous approach,
guarantees that we will always have the entire .text section
available for re-mapping.

Signed-off-by: @gabrielschulhof

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines


          src: retrieve .text coordinates for large pages

31f32ba

After locating the in-memory base address of the Node.js executable
using `dl_iterate_phdr(3)` we open the on-disk executable file and
extract the address and size of the `.text` section. We then add the
in-memory base address to the offset to locate the `.text` section in
memory.

This approach removes the need for a symbol to tell us where the
`.text` section is located, and, unlike the previous approach,
guarantees that we will always have the entire `.text` section
available for re-mapping.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>

gabrielschulhof requested review from addaleax, bnoordhuis, devnexen, jasnell and lundibundi

April 28, 2020 05:21

nodejs-github-bot added build c++ labels

devnexen approved these changes

View reviewed changes

Collaborator

nodejs-github-bot commented Apr 28, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31049/

bnoordhuis requested changes

View reviewed changes

Member

bnoordhuis left a comment

I don't think this is a good idea for two reasons:

Won't work when /proc isn't mounted.
Does a lot of extra file I/O at startup.

The second one in particular is a deal breaker.

src/large_pages/node_large_page.cc Outdated

    
                class_name(const class_name&) = delete;       \

                class_name(class_name&&) = delete;            \

                void operator= (const class_name&) = delete;  \

                void operator= (const class_name&&) = delete

Member

bnoordhuis Apr 28, 2020

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?

src/large_pages/node_large_page.cc Outdated

    
                  struct stat file_info;

                  if (stat(fname, &file_info) == -1) goto fail;

                  fd_ = open(fname, O_RDONLY);

Member

bnoordhuis Apr 28, 2020

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. :-)

src/large_pages/node_large_page.cc

    
                FORCE_INLINE ~MappedFilePointer() {

                  if (fd_ == -1) return;

                  if (close(fd_) == 0) return;

                  PrintSystemError(errno);

Member

bnoordhuis Apr 28, 2020

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.

Contributor Author

gabrielschulhof Apr 28, 2020

I added this.

src/large_pages/node_large_page.cc Outdated

    
                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") {

Member

bnoordhuis Apr 28, 2020

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.

src/large_pages/node_large_page.cc

    
                  }

                ElfW(Shdr) text_section;

              #if defined(__linux__)

                dl_params.exename = "/proc/self/exe";

Member

bnoordhuis Apr 28, 2020

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


          address review comments

Contributor Author

gabrielschulhof commented Apr 28, 2020

@bnoordhuis I checked the startup time wrt. master and it does not seem to be different at all with this change. This feature used to rely on reading /proc/self/maps instead of dl_iterate_phdr(). There was file I/O and reliance on /proc in that. IMO removing the complicated build process and the groping in the dark of the segment with heuristics like

node/src/large_pages/node_large_page.cc

Lines 163 to 168 in f64c640

    
           if (dl_params->reference_sym >= start && 
        
               dl_params->reference_sym <= end) { 
        
             dl_params->start = start; 
        
             dl_params->end = end; 
        
             return 1; 
        
           }

and

node/src/large_pages/node_large_page.cc

Lines 205 to 209 in f64c640

    
           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; 
        
           }

may be worth the renewed reliance on /proc.


          heed the linter

2525cb9

Contributor Author

gabrielschulhof commented Apr 28, 2020

@bnoordhuis I also added a Debug() in MappedFilePointer::Reset in case it fails because of a lack of /proc.

Member

bnoordhuis commented Apr 28, 2020

I checked the startup time wrt. master and it does not seem to be different at all with this change.

The kernel reads the ELF header on startup so it's often in the page cache, but when it's not and you're running node off your Raspberry Pi's SD card, you're going to notice.

This feature used to rely on reading /proc/self/maps instead of dl_iterate_phdr(). There was file I/O and reliance on /proc in that.

procfs is a virtual fs though, there's no actual disk I/O taking place.

Contributor Author

gabrielschulhof commented May 1, 2020

@bnoordhuis fair enough.

gabrielschulhof closed this

gabrielschulhof deleted the large-pages-read-elf branch

January 28, 2021 00:14

gabrielschulhof restored the large-pages-read-elf branch

January 28, 2021 05:40

gabrielschulhof deleted the large-pages-read-elf branch

February 3, 2021 07:17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis requested changes

jasnell Awaiting requested review from jasnell

addaleax Awaiting requested review from addaleax

lundibundi Awaiting requested review from lundibundi

+1 more reviewer

devnexen devnexen approved these changes

Labels

build c++