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

Small fixes from living unusually #265

Merged
merged 3 commits into from
Dec 16, 2020
Merged

Conversation

nwf
Copy link
Collaborator

@nwf nwf commented Dec 14, 2020

Riding around on a POWER Linux box brings out corner cases.

@nwf nwf requested review from mjp41 and davidchisnall December 14, 2020 19:12
@nwf nwf force-pushed the 202012-misc-fixes branch from c6ce72a to a52cb44 Compare December 14, 2020 19:35
@@ -47,12 +59,14 @@ namespace snmalloc
// Use flat map is under a single node.
# define SNMALLOC_MAX_FLATPAGEMAP_SIZE PAGEMAP_NODE_SIZE
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to change

#  define SNMALLOC_MAX_FLATPAGEMAP_SIZE PAGEMAP_NODE_SIZE

to

#  define SNMALLOC_MAX_FLATPAGEMAP_SIZE \
  pal_supports<LazyCommit> ? .... : PAGEMAP_NODE_SIZE

Where we choose ... suitably.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With default chunk size (i.e., neither USE_SMALL_CHUNKS nor USE_LARGE_CHUNKS), SUPERSLAB_BITS is 20. With a 48-bit VA, that means a flat chunkmap pagemap is 256 MiB. Is that a reasonable choice for ...?

Copy link
Member

Choose a reason for hiding this comment

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

Probably, I would say going above 2^32 causes a lot of issues, so below that maybe 1GiB is the max?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Permitting 512M will break src/test/func/memory/memory.cc; we could adjust the test, tho'.

Copy link
Member

Choose a reason for hiding this comment

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

Either is fine. Just document the choice.

@nwf nwf force-pushed the 202012-misc-fixes branch 2 times, most recently from ee991ec to 87bd7b5 Compare December 15, 2020 16:07
This had not been observed as an issue prior to
923705e because CMakeLists.txt had, until
then, been using EQUAL, not STREQUAL, to test for oe (and to then enable
USE_SMALL_CHUNKS).  This test would fail, and so the default SLAB_SIZE was
used.  Absent this min operation, the use of a whole page on a 64KiB page
causes a crash when using the largest medium size class, as, ultimately, size
classes are not based on page sizes, and so committing a whole page to the
header leaves too little room for that class.

See also 3d3b048.
@nwf nwf force-pushed the 202012-misc-fixes branch from 87bd7b5 to 912d8a6 Compare December 15, 2020 16:08
Presently, our flat pagemap can be configured to take...

                   32-bit AS    48-bit AS
USE_SMALL_CHUNKS     16 KiB        1 GiB
default               4 KiB      256 MiB
USE_LARGE_CHUNKS    256   B       16 MiB

At 1 GiB, we're already past the 512 MiB threshold imposed when
src/test/func/memory/memory.cc, when configured to TEST_LIMITED, probes the
effect of rlimit.

Instead, restrict flat pagemaps to at most 256 MiB of AS by default (override
by defining SNMALLOC_MAX_FLATPAGEMAP_SIZE), which forces the USE_SMALL_CHUNKS &
48-bit AS configuration to use the tree-based version.

While here, rename USE_FLATPAGEMAP to CHUNKMAP_USE_FLATPAGEMAP.
@nwf nwf force-pushed the 202012-misc-fixes branch from 912d8a6 to 42a7a3a Compare December 15, 2020 21:58
@nwf nwf mentioned this pull request Dec 16, 2020
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

LGTM

@mjp41 mjp41 merged commit e9ed219 into microsoft:master Dec 16, 2020
@nwf nwf deleted the 202012-misc-fixes branch December 16, 2020 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants