iframe-proxy

gabrielschulhof · 2020-03-20T22:06:47Z

Introduce OnScopeLeave handler for cleaning up mmap()ed range(s).
Factor out failure scenario at the bottom of the function with
fail label for use with goto.
Do not allocate temporary range (nmem) on FreeBSD, because it is
not used.

The intention is that the steps involved in re-mapping to large pages
become more clearly visible.

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

Checklist

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

gabrielschulhof · 2020-03-24T20:35:51Z

* Introduce `OnScopeLeave` handler for cleaning up mmap()ed range(s). * Factor out failure scenario at the bottom of the function with `fail` label for use with `goto`. * Do not allocate temporary range (`nmem`) on FreeBSD, because it is not used. The intention is that the steps involved in re-mapping to large pages become more clearly visible. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot · 2020-03-24T20:40:05Z

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

gabrielschulhof · 2020-03-24T20:49:14Z

@devnexen is there a combination involving FreeBSD under which the mapping actually works?

gabrielschulhof · 2020-03-24T20:54:30Z

@devnexen I tried building on test-digitalocean-freebsd11-x64-2 and on FreeBSD 12.1 from https://www.osboxes.org/freebsd/ and they both refuse to re-map because of the requirement that the region to map not precede the mapping function:

node/src/large_pages/node_large_page.cc

Lines 454 to 457 in 066bdec

    
           #if defined(__FreeBSD__) 
        
             if (r.from < reinterpret_cast<void*>(&MoveTextRegionToLargePages)) 
        
               return -1; 
        
           #endif

codecov-io · 2020-03-25T03:19:35Z

Codecov Report

Merging #32396 into master will increase coverage by 0.30%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #32396      +/-   ##
==========================================
+ Coverage   97.16%   97.46%   +0.30%     
==========================================
  Files         197      195       -2     
  Lines       65781    65701      -80     
==========================================
+ Hits        63914    64037     +123     
+ Misses       1867     1664     -203

Impacted Files	Coverage Δ
lib/events.js	`95.38% <0.00%> (-4.62%)`	⬇️
lib/net.js	`98.18% <0.00%> (-0.29%)`	⬇️
lib/internal/modules/esm/loader.js	`88.57% <0.00%> (-0.18%)`	⬇️
lib/_http_server.js	`98.27% <0.00%> (-0.12%)`	⬇️
lib/_tls_wrap.js	`95.45% <0.00%> (-0.02%)`	⬇️
lib/zlib.js	`98.36% <0.00%> (-0.01%)`	⬇️
...l/bootstrap/switches/does_not_own_process_state.js
lib/internal/constants.js
lib/internal/console/global.js
lib/internal/bootstrap/environment.js	`100.00% <0.00%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c160073...470d2d2. Read the comment docs.

nodejs-github-bot · 2020-03-25T04:49:33Z

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

gabrielschulhof · 2020-03-26T16:16:13Z

@devnexen NM - I got it to work.

nodejs-github-bot · 2020-03-26T16:45:33Z

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

jasnell · 2020-03-27T21:47:08Z

+    if (nmem != nullptr && nmem != MAP_FAILED && munmap(nmem, size) == -1)
+      PrintSystemError(errno);
+    if (tmem != nullptr && tmem != MAP_FAILED && munmap(tmem, size) == -1)
+      PrintSystemError(errno);


Suggested change

if (nmem != nullptr && nmem != MAP_FAILED && munmap(nmem, size) == -1)

PrintSystemError(errno);

if (tmem != nullptr && tmem != MAP_FAILED && munmap(tmem, size) == -1)

PrintSystemError(errno);

if ((nmem != nullptr && nmem != MAP_FAILED && munmap(nmem, size) == -1) ||

(tmem != nullptr && tmem != MAP_FAILED && munmap(tmem, size) == -1)) {

PrintSystemError(errno);

}

Alternatively, it may be a bit cleaner to use a custom smart pointer with a deleter for these...

e.g. bit more verbose...

struct Mmap { void* mem = nullptr; size_t size = 0; Mmap(void* start, size_t size, int prot, int flags) { mem = mmap(start, size, prot, flags, -1, 0); } }; struct MmapDeleter { void operator()(Mmap* ptr) const noexcept { if (ptr.mem != nullptr && ptr.mem != MAP_FAILED && munmap(ptr.mem, ptr.size) == -1) PrintSystemError(errno); } }; using MmapPointer = std::unique_ptr<Mmap, MmapDeleter>;

then...

MmapPointer nmem; MmapPointer tmem; // ... size_t size = r.to - r.from; // ... nmem = std::make_unique(nullptr, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS); if (nmem.mem == MAP_FAILED) goto fail; // ... tmem = std::make_unique(start, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED); if (tmem.mem == MAP_FAILED) goto fail;

gabrielschulhof · 2020-03-28T02:26:54Z

@jasnell I've left #32532 as a todo for myself.

gabrielschulhof · 2020-03-28T02:45:08Z

Landed in fa3fd78.

* Introduce `OnScopeLeave` handler for cleaning up mmap()ed range(s). * Factor out failure scenario at the bottom of the function with `fail` label for use with `goto`. * Do not allocate temporary range (`nmem`) on FreeBSD, because it is not used. The intention is that the steps involved in re-mapping to large pages become more clearly visible. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: #32396 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

targos · 2020-04-22T16:26:13Z

* Introduce `OnScopeLeave` handler for cleaning up mmap()ed range(s). * Factor out failure scenario at the bottom of the function with `fail` label for use with `goto`. * Do not allocate temporary range (`nmem`) on FreeBSD, because it is not used. The intention is that the steps involved in re-mapping to large pages become more clearly visible. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: nodejs#32396 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

* Introduce `OnScopeLeave` handler for cleaning up mmap()ed range(s). * Factor out failure scenario at the bottom of the function with `fail` label for use with `goto`. * Do not allocate temporary range (`nmem`) on FreeBSD, because it is not used. The intention is that the steps involved in re-mapping to large pages become more clearly visible. Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com> Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl> PR-URL: #32396 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: David Carlier <devnexen@gmail.com>

gabrielschulhof added the wip Issues and PRs that are still a work in progress. label Mar 20, 2020

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 20, 2020

gabrielschulhof mentioned this pull request Mar 20, 2020

May I please have access to one FreeBSD and one OSX machine? nodejs/build#2240

Closed

bnoordhuis reviewed Mar 21, 2020

View reviewed changes

Comment thread src/large_pages/node_large_page.cc Outdated

bnoordhuis approved these changes Mar 22, 2020

View reviewed changes

gabrielschulhof force-pushed the large-pages-simplify-code branch from 45c73c3 to 9f2adb5 Compare March 24, 2020 06:16

gabrielschulhof changed the title ~~[WIP] src: simplify large pages mapping code~~ src: simplify large pages mapping code Mar 24, 2020

gabrielschulhof removed the wip Issues and PRs that are still a work in progress. label Mar 24, 2020

gabrielschulhof force-pushed the large-pages-simplify-code branch from 9f2adb5 to 470d2d2 Compare March 24, 2020 20:38

gabrielschulhof requested review from devnexen and lundibundi March 24, 2020 20:40

devnexen approved these changes Mar 27, 2020

View reviewed changes

jasnell reviewed Mar 27, 2020

View reviewed changes

gabrielschulhof mentioned this pull request Mar 28, 2020

Convert mmap()s to smart pointers #32532

Closed

gabrielschulhof closed this Mar 28, 2020

targos mentioned this pull request Apr 13, 2020

v13.13.0 release proposal #32813

Merged

gabrielschulhof deleted the large-pages-simplify-code branch April 21, 2020 02:58

targos added the backport-blocked-v12.x label Apr 22, 2020

targos removed the backport-blocked-v12.x label Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Uh oh!

Conversation

gabrielschulhof commented Mar 20, 2020

Checklist

Uh oh!

Uh oh!

gabrielschulhof commented Mar 24, 2020

Uh oh!

nodejs-github-bot commented Mar 24, 2020

Uh oh!

gabrielschulhof commented Mar 24, 2020

Uh oh!

gabrielschulhof commented Mar 24, 2020

Uh oh!

codecov-io commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Mar 25, 2020

Uh oh!

gabrielschulhof commented Mar 26, 2020

Uh oh!

nodejs-github-bot commented Mar 26, 2020

Uh oh!

jasnell Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

gabrielschulhof commented Mar 28, 2020

Uh oh!

gabrielschulhof commented Mar 28, 2020

Uh oh!

targos commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Mar 25, 2020 •

edited

Loading