OpenBSD build fix proposal.

[devnexen]

Seems on this system pages are not discarded in due time
thus some of unit tests are failing, leading to memory corruption.
Marking those pages not accessible instead and test pass now.

[mjp41]

Don't you need the converse to be set in notify_using?

In particular, we have code to do this. It was added to simulate the Windows like commit model on Posix:

snmalloc/src/pal/pal_posix.h

Lines 137 to 139 in 17d5a3e

	#ifdef USE_POSIX_COMMIT_CHECKS
	mprotect(p, size, PROT_NONE);
	#else

snmalloc/src/pal/pal_posix.h

Lines 158 to 160 in 17d5a3e

	#ifdef USE_POSIX_COMMIT_CHECKS
	mprotect(p, size, PROT_READ | PROT_WRITE);
	#else

It seems very strange that this is would be required. Is there an OS bug we can track for this?

[devnexen]

Don't you need the converse to be set in notify_using?

I did not really need to but for consistency sure.

In particular, we have code to do this. It was added to simulate the Windows like commit model on Posix:

snmalloc/src/pal/pal_posix.h

Lines 137 to 139 in 17d5a3e

	#ifdef USE_POSIX_COMMIT_CHECKS
	mprotect(p, size, PROT_NONE);
	#else

snmalloc/src/pal/pal_posix.h

Lines 158 to 160 in 17d5a3e

	#ifdef USE_POSIX_COMMIT_CHECKS
	mprotect(p, size, PROT_READ | PROT_WRITE);
	#else

It seems very strange that this is would be required. Is there an OS bug we can track for this?

OpenBSD does not have bug tracker per see, just old school mailing list, I did not find any bug related to however was reading the uvm map code https://github.com/openbsd/src/blob/2d2e8ab08946096eec777725afff21569594ceef/sys/uvm/uvm_map.c#L4704 seems should have done it. However with some test (e.g. perf-contention-*) got some use after free issues.

[mjp41]

What kind of CPU are you using for the tests?

[devnexen]

Virtualized environement, usual x86 64 bits host. Note that the FreeBSD guest works ok with same resources allocation.

[davidchisnall]

I don't think we should be seeing corruption even if the pages are not returned fast enough, at worst we should be seeing too much physical memory usage. I wonder if there's a bug in the MADV_FREE implementation on OpenBSD? We recently found some in FreeBSD, which involve subtle ordering issues in the VM subsystem.

[devnexen]

Or maybe MADV_FREE has not same semantic as other systems ?

[devnexen]

Digged out an old thread somewhere https://groups.google.com/g/golang-dev/c/oLlj0ssS4Vg

[davidchisnall]

That thread seems to be pointing to a FreeBSD bug that @nwf recently found and diagnosed that doing MADV_FREE on pages backed by shadow objects will sometimes give you the pre-CoW copy back and not the current version or zero. That shouldn't matter for snmalloc, because we don't rely on the contents of those pages being preserved or zeroed, we're happy to get garbage back. I suspect OpenBSD has the same bug because this looks like something that is fairly intrinsic to the 4BSD VM subsystem, but that shouldn't matter for us.

[devnexen]

perf-contention-16 for example aborts at mem/superslab.h, line 110 with master branch.

[mjp41]

@devnexen thank you for the extra information. I believe the problem is the following.

• In the 16MiB configuration, there are 256 slabs in a superslab (chunk), the meta-data for a slab is 16 bytes, and thus there is 4KiB of meta data. There is a small amount of meta-data at the superslab level, so the meta-data covers slightly more than the first page.
• The first page is never handed back to the OS as it is used for the mpmcstack of free chunks.
• Subsequent pages can be handed back to the OS.

This means the very end of the meta-data can become zeroed. Getting the original is fine, but getting random stuff is very bad. I am not convinced being zeroed is right, but it is not tripping any debug tests on Windows.

I think this is actually a "bug" across platforms. If zeros are fine, then this would not exhibit on any correctly implemented platform. If zeros are not fine, then care is needed here.

Can you confirm, does this fail on the 1MiB configuration? Or only 16MiB? The 1MiB would not have this issue, as the meta-data fits into the first 4KiB.

The original assumption with this code was all the meta-data was in the first page, but #102 broke that assumption.

The simplest fix would be to move the initialisation code to always occur:

snmalloc/src/mem/superslab.h

Lines 86 to 100 in 8990c34

	if (kind != Fresh)
	{
	// If this wasn't previously Fresh, we need to zero some things.
	used = 0;
	for (size_t i = 0; i < SLAB_COUNT; i++)
	{
	new (&(meta[i])) Metaslab();
	}
	}
	// If this wasn't previously a Superslab, we need to set up the
	// header.
	kind = Super;
	// Point head at the first non-short slab.
	head = 1;

[devnexen]

Indeed I purposely only highlighted the 16MB case, the 1MB's is fine.

[mjp41]

@devnexen I have replicated this on WSL by just simulating the zeroing immediately. I will fix this as high-priority.

[mjp41]

#259 should fix this on OpenBSD. It also adds checks on Linux, so we don't regress this behaviour.