Skip to content
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.

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

src: large pages fix FreeBSD fix region size #28735

Closed
wants to merge 1 commit into from

Conversation

@devnexen
Copy link
Contributor

commented Jul 17, 2019

Makes the size aligned to huge page size.

@devnexen devnexen force-pushed the devnexen:fbsd_large_page_sz_aligned branch from cfe1c8f to d3db59d Jul 17, 2019

@devnexen devnexen changed the title src: large pages fix FreeBSD fix region size. src: large pages fix FreeBSD fix region size Jul 17, 2019

@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Is this @nodejs/platform-freebsd specific? I labelled it as such, based on the commit message, please comment if that was incorrect.

The commit message/PR could use some more description. It appears you have switched the size calculation from being before the alignment of the start/end, to after. Does this have an effect which can be tested?

@sam-github sam-github added the freebsd label Jul 22, 2019

@devnexen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@sam-github it definitely is :-) It has an effect only of the number of large pages now it is correct since it comes from the aligned lower and upper boundary of the address.

src: large pages fix FreeBSD fix region size
Makes the size aligned to huge page size by
calculating it from the aligned lower and upper
boundary of the executable address.

@devnexen devnexen force-pushed the devnexen:fbsd_large_page_sz_aligned branch from d3db59d to 2b1d649 Jul 22, 2019

@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

commit message looks good, hopefully someone on @nodejs/platform-freebsd can confirm its correct.

Is there a way to do a before and after test, even if its manual, to see what went wrong before, and what is working now?

@nodejs-github-bot

This comment has been minimized.

@sam-github sam-github referenced this pull request Jul 22, 2019
0 of 2 tasks complete
@devnexen

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

In fact I was just curious to print the number of size and total huge page and realised where the issue came from, since we divide by the size of huge page that quite "jump to the face". I got few thumb ups I guess I m not far from the truth :-)

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

This needs a review. /ping @nodejs/build @nodejs/collaborators

@Trott Trott added the review wanted label Jul 25, 2019

@rvagg
rvagg approved these changes Jul 26, 2019
Copy link
Member

left a comment

I'm going to rubber-stamp this one because (a) it's contained within a FreeBSD-only block and (b) @devnexen is the original author of this code.

@Trott
Trott approved these changes Jul 30, 2019
Copy link
Member

left a comment

Rubber-stamp LGTM per @rvagg's comment.

@Trott

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Landed in de88d6c

@Trott Trott closed this Jul 30, 2019

Trott added a commit to Trott/io.js that referenced this pull request Jul 30, 2019
src: large pages fix FreeBSD fix region size
Makes the size aligned to huge page size by
calculating it from the aligned lower and upper
boundary of the executable address.

PR-URL: nodejs#28735
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@Trott Trott removed the review wanted label Jul 30, 2019

targos added a commit that referenced this pull request Aug 2, 2019
src: large pages fix FreeBSD fix region size
Makes the size aligned to huge page size by
calculating it from the aligned lower and upper
boundary of the executable address.

PR-URL: #28735
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR referenced this pull request Aug 6, 2019
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: large pages fix FreeBSD fix region size
Makes the size aligned to huge page size by
calculating it from the aligned lower and upper
boundary of the executable address.

PR-URL: nodejs#28735
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
JeniaBR added a commit to JeniaBR/node that referenced this pull request Sep 11, 2019
src: large pages fix FreeBSD fix region size
Makes the size aligned to huge page size by
calculating it from the aligned lower and upper
boundary of the executable address.

PR-URL: nodejs#28735
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.