cache_bin: Separate out local and remote accesses.

This fixes an incorrect debug-mode assert:
- T1 starts an arena stats update and reads stack_head from another thread's
  cache bin, when that cache bin has 1 item in it.
- T2 allocates from that cache bin.  The cache_bin's stack_head now points to a
  NULL pointer, since the cache bin is empty.
- T1 Re-reads the cache_bin's stack_head to perform an assertion check (since it
  previously saw that the bin was empty, whatever stack_head points to should be
  non-NULL).
This commit is contained in:
David Goldblatt 2021-01-07 13:22:08 -08:00 committed by David Goldblatt
parent 14d689c0f9
commit a011c4c22d
5 changed files with 70 additions and 36 deletions

View File

@ -167,20 +167,50 @@ cache_bin_diff(cache_bin_t *bin, uint16_t earlier, uint16_t later) {
return later - earlier; return later - earlier;
} }
/* Number of items currently cached in the bin, without checking ncached_max. */ /*
* Number of items currently cached in the bin, without checking ncached_max.
* We require specifying whether or not the request is racy or not (i.e. whether
* or not concurrent modifications are possible).
*/
static inline cache_bin_sz_t static inline cache_bin_sz_t
cache_bin_ncached_get_internal(cache_bin_t *bin) { cache_bin_ncached_get_internal(cache_bin_t *bin, bool racy) {
cache_bin_sz_t diff = cache_bin_diff(bin, cache_bin_sz_t diff = cache_bin_diff(bin,
(uint16_t)(uintptr_t)bin->stack_head, bin->low_bits_empty); (uint16_t)(uintptr_t)bin->stack_head, bin->low_bits_empty);
cache_bin_sz_t n = diff / sizeof(void *); cache_bin_sz_t n = diff / sizeof(void *);
assert(n == 0 || *(bin->stack_head) != NULL); /*
* We have undefined behavior here; if this function is called from the
* arena stats updating code, then stack_head could change from the
* first line to the next one. Morally, these loads should be atomic,
* but compilers won't currently generate comparisons with in-memory
* operands against atomics, and these variables get accessed on the
* fast paths. This should still be "safe" in the sense of generating
* the correct assembly for the foreseeable future, though.
*/
assert(n == 0 || *(bin->stack_head) != NULL || racy);
return n; return n;
} }
/* Number of items currently cached in the bin, with checking ncached_max. */ /*
* Number of items currently cached in the bin, with checking ncached_max. The
* caller must know that no concurrent modification of the cache_bin is
* possible.
*/
static inline cache_bin_sz_t static inline cache_bin_sz_t
cache_bin_ncached_get(cache_bin_t *bin, cache_bin_info_t *info) { cache_bin_ncached_get_local(cache_bin_t *bin, cache_bin_info_t *info) {
cache_bin_sz_t n = cache_bin_ncached_get_internal(bin); cache_bin_sz_t n = cache_bin_ncached_get_internal(bin,
/* racy */ false);
assert(n <= cache_bin_info_ncached_max(info));
return n;
}
/*
* Obtain a racy view of the number of items currently in the cache bin, in the
* presence of possible concurrent modifications.
*/
static inline cache_bin_sz_t
cache_bin_ncached_get_remote(cache_bin_t *bin, cache_bin_info_t *info) {
cache_bin_sz_t n = cache_bin_ncached_get_internal(bin,
/* racy */ true);
assert(n <= cache_bin_info_ncached_max(info)); assert(n <= cache_bin_info_ncached_max(info));
return n; return n;
} }
@ -208,7 +238,7 @@ cache_bin_empty_position_get(cache_bin_t *bin) {
*/ */
static inline void static inline void
cache_bin_assert_empty(cache_bin_t *bin, cache_bin_info_t *info) { cache_bin_assert_empty(cache_bin_t *bin, cache_bin_info_t *info) {
assert(cache_bin_ncached_get(bin, info) == 0); assert(cache_bin_ncached_get_local(bin, info) == 0);
assert(cache_bin_empty_position_get(bin) == bin->stack_head); assert(cache_bin_empty_position_get(bin) == bin->stack_head);
} }
@ -228,7 +258,7 @@ static inline cache_bin_sz_t
cache_bin_low_water_get(cache_bin_t *bin, cache_bin_info_t *info) { cache_bin_low_water_get(cache_bin_t *bin, cache_bin_info_t *info) {
cache_bin_sz_t low_water = cache_bin_low_water_get_internal(bin); cache_bin_sz_t low_water = cache_bin_low_water_get_internal(bin);
assert(low_water <= cache_bin_info_ncached_max(info)); assert(low_water <= cache_bin_info_ncached_max(info));
assert(low_water <= cache_bin_ncached_get(bin, info)); assert(low_water <= cache_bin_ncached_get_local(bin, info));
cache_bin_assert_earlier(bin, (uint16_t)(uintptr_t)bin->stack_head, cache_bin_assert_earlier(bin, (uint16_t)(uintptr_t)bin->stack_head,
bin->low_bits_low_water); bin->low_bits_low_water);
@ -247,7 +277,7 @@ cache_bin_low_water_set(cache_bin_t *bin) {
static inline void static inline void
cache_bin_low_water_adjust(cache_bin_t *bin) { cache_bin_low_water_adjust(cache_bin_t *bin) {
if (cache_bin_ncached_get_internal(bin) if (cache_bin_ncached_get_internal(bin, /* racy */ false)
< cache_bin_low_water_get_internal(bin)) { < cache_bin_low_water_get_internal(bin)) {
cache_bin_low_water_set(bin); cache_bin_low_water_set(bin);
} }
@ -319,7 +349,7 @@ cache_bin_alloc(cache_bin_t *bin, bool *success) {
JEMALLOC_ALWAYS_INLINE cache_bin_sz_t JEMALLOC_ALWAYS_INLINE cache_bin_sz_t
cache_bin_alloc_batch(cache_bin_t *bin, size_t num, void **out) { cache_bin_alloc_batch(cache_bin_t *bin, size_t num, void **out) {
size_t n = cache_bin_ncached_get_internal(bin); size_t n = cache_bin_ncached_get_internal(bin, /* racy */ false);
if (n > num) { if (n > num) {
n = num; n = num;
} }
@ -416,7 +446,7 @@ static inline void
cache_bin_init_ptr_array_for_flush(cache_bin_t *bin, cache_bin_info_t *info, cache_bin_init_ptr_array_for_flush(cache_bin_t *bin, cache_bin_info_t *info,
cache_bin_ptr_array_t *arr, cache_bin_sz_t nflush) { cache_bin_ptr_array_t *arr, cache_bin_sz_t nflush) {
arr->ptr = cache_bin_empty_position_get(bin) - 1; arr->ptr = cache_bin_empty_position_get(bin) - 1;
assert(cache_bin_ncached_get(bin, info) == 0 assert(cache_bin_ncached_get_local(bin, info) == 0
|| *arr->ptr != NULL); || *arr->ptr != NULL);
} }
@ -437,7 +467,7 @@ cache_bin_ptr_array_set(cache_bin_ptr_array_t *arr, cache_bin_sz_t n, void *p) {
static inline void static inline void
cache_bin_finish_flush(cache_bin_t *bin, cache_bin_info_t *info, cache_bin_finish_flush(cache_bin_t *bin, cache_bin_info_t *info,
cache_bin_ptr_array_t *arr, cache_bin_sz_t nflushed) { cache_bin_ptr_array_t *arr, cache_bin_sz_t nflushed) {
unsigned rem = cache_bin_ncached_get(bin, info) - nflushed; unsigned rem = cache_bin_ncached_get_local(bin, info) - nflushed;
memmove(bin->stack_head + nflushed, bin->stack_head, memmove(bin->stack_head + nflushed, bin->stack_head,
rem * sizeof(void *)); rem * sizeof(void *));
bin->stack_head = bin->stack_head + nflushed; bin->stack_head = bin->stack_head + nflushed;

View File

@ -150,7 +150,7 @@ arena_stats_merge(tsdn_t *tsdn, arena_t *arena, unsigned *nthreads,
for (szind_t i = 0; i < nhbins; i++) { for (szind_t i = 0; i < nhbins; i++) {
cache_bin_t *cache_bin = &descriptor->bins[i]; cache_bin_t *cache_bin = &descriptor->bins[i];
astats->tcache_bytes += astats->tcache_bytes +=
cache_bin_ncached_get(cache_bin, cache_bin_ncached_get_remote(cache_bin,
&tcache_bin_info[i]) * sz_index2size(i); &tcache_bin_info[i]) * sz_index2size(i);
} }
} }
@ -767,7 +767,7 @@ void
arena_cache_bin_fill_small(tsdn_t *tsdn, arena_t *arena, arena_cache_bin_fill_small(tsdn_t *tsdn, arena_t *arena,
cache_bin_t *cache_bin, cache_bin_info_t *cache_bin_info, szind_t binind, cache_bin_t *cache_bin, cache_bin_info_t *cache_bin_info, szind_t binind,
const unsigned nfill) { const unsigned nfill) {
assert(cache_bin_ncached_get(cache_bin, cache_bin_info) == 0); assert(cache_bin_ncached_get_local(cache_bin, cache_bin_info) == 0);
const bin_info_t *bin_info = &bin_infos[binind]; const bin_info_t *bin_info = &bin_infos[binind];

View File

@ -83,7 +83,7 @@ cache_bin_init(cache_bin_t *bin, cache_bin_info_t *info, void *alloc,
bin->low_bits_empty = (uint16_t)(uintptr_t)empty_position; bin->low_bits_empty = (uint16_t)(uintptr_t)empty_position;
assert(cache_bin_diff(bin, bin->low_bits_full, assert(cache_bin_diff(bin, bin->low_bits_full,
(uint16_t)(uintptr_t) bin->stack_head) == bin_stack_size); (uint16_t)(uintptr_t) bin->stack_head) == bin_stack_size);
assert(cache_bin_ncached_get(bin, info) == 0); assert(cache_bin_ncached_get_local(bin, info) == 0);
assert(cache_bin_empty_position_get(bin) == empty_position); assert(cache_bin_empty_position_get(bin) == empty_position);
assert(bin_stack_size > 0 || empty_position == full_position); assert(bin_stack_size > 0 || empty_position == full_position);

View File

@ -125,7 +125,7 @@ tcache_gc_small(tsd_t *tsd, tcache_slow_t *tcache_slow, tcache_t *tcache,
assert(szind < SC_NBINS); assert(szind < SC_NBINS);
cache_bin_t *cache_bin = &tcache->bins[szind]; cache_bin_t *cache_bin = &tcache->bins[szind];
cache_bin_sz_t ncached = cache_bin_ncached_get(cache_bin, cache_bin_sz_t ncached = cache_bin_ncached_get_local(cache_bin,
&tcache_bin_info[szind]); &tcache_bin_info[szind]);
cache_bin_sz_t low_water = cache_bin_low_water_get(cache_bin, cache_bin_sz_t low_water = cache_bin_low_water_get(cache_bin,
&tcache_bin_info[szind]); &tcache_bin_info[szind]);
@ -159,7 +159,7 @@ tcache_gc_large(tsd_t *tsd, tcache_slow_t *tcache_slow, tcache_t *tcache,
/* Like the small GC; flush 3/4 of untouched items. */ /* Like the small GC; flush 3/4 of untouched items. */
assert(szind >= SC_NBINS); assert(szind >= SC_NBINS);
cache_bin_t *cache_bin = &tcache->bins[szind]; cache_bin_t *cache_bin = &tcache->bins[szind];
cache_bin_sz_t ncached = cache_bin_ncached_get(cache_bin, cache_bin_sz_t ncached = cache_bin_ncached_get_local(cache_bin,
&tcache_bin_info[szind]); &tcache_bin_info[szind]);
cache_bin_sz_t low_water = cache_bin_low_water_get(cache_bin, cache_bin_sz_t low_water = cache_bin_low_water_get(cache_bin,
&tcache_bin_info[szind]); &tcache_bin_info[szind]);
@ -289,7 +289,7 @@ tcache_bin_flush_impl(tsd_t *tsd, tcache_t *tcache, cache_bin_t *cache_bin,
} else { } else {
assert(binind < nhbins); assert(binind < nhbins);
} }
cache_bin_sz_t ncached = cache_bin_ncached_get(cache_bin, cache_bin_sz_t ncached = cache_bin_ncached_get_local(cache_bin,
&tcache_bin_info[binind]); &tcache_bin_info[binind]);
assert((cache_bin_sz_t)rem <= ncached); assert((cache_bin_sz_t)rem <= ncached);
arena_t *tcache_arena = tcache_slow->arena; arena_t *tcache_arena = tcache_slow->arena;

View File

@ -6,14 +6,15 @@ do_fill_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
cache_bin_sz_t nfill_succeed) { cache_bin_sz_t nfill_succeed) {
bool success; bool success;
void *ptr; void *ptr;
assert_true(cache_bin_ncached_get(bin, info) == 0, ""); assert_true(cache_bin_ncached_get_local(bin, info) == 0, "");
CACHE_BIN_PTR_ARRAY_DECLARE(arr, nfill_attempt); CACHE_BIN_PTR_ARRAY_DECLARE(arr, nfill_attempt);
cache_bin_init_ptr_array_for_fill(bin, info, &arr, nfill_attempt); cache_bin_init_ptr_array_for_fill(bin, info, &arr, nfill_attempt);
for (cache_bin_sz_t i = 0; i < nfill_succeed; i++) { for (cache_bin_sz_t i = 0; i < nfill_succeed; i++) {
arr.ptr[i] = &ptrs[i]; arr.ptr[i] = &ptrs[i];
} }
cache_bin_finish_fill(bin, info, &arr, nfill_succeed); cache_bin_finish_fill(bin, info, &arr, nfill_succeed);
expect_true(cache_bin_ncached_get(bin, info) == nfill_succeed, ""); expect_true(cache_bin_ncached_get_local(bin, info) == nfill_succeed,
"");
cache_bin_low_water_set(bin); cache_bin_low_water_set(bin);
for (cache_bin_sz_t i = 0; i < nfill_succeed; i++) { for (cache_bin_sz_t i = 0; i < nfill_succeed; i++) {
@ -24,7 +25,7 @@ do_fill_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
expect_true(cache_bin_low_water_get(bin, info) expect_true(cache_bin_low_water_get(bin, info)
== nfill_succeed - i - 1, ""); == nfill_succeed - i - 1, "");
} }
expect_true(cache_bin_ncached_get(bin, info) == 0, ""); expect_true(cache_bin_ncached_get_local(bin, info) == 0, "");
expect_true(cache_bin_low_water_get(bin, info) == 0, ""); expect_true(cache_bin_low_water_get(bin, info) == 0, "");
} }
@ -32,7 +33,7 @@ static void
do_flush_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs, do_flush_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
cache_bin_sz_t nfill, cache_bin_sz_t nflush) { cache_bin_sz_t nfill, cache_bin_sz_t nflush) {
bool success; bool success;
assert_true(cache_bin_ncached_get(bin, info) == 0, ""); assert_true(cache_bin_ncached_get_local(bin, info) == 0, "");
for (cache_bin_sz_t i = 0; i < nfill; i++) { for (cache_bin_sz_t i = 0; i < nfill; i++) {
success = cache_bin_dalloc_easy(bin, &ptrs[i]); success = cache_bin_dalloc_easy(bin, &ptrs[i]);
@ -46,8 +47,9 @@ do_flush_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
} }
cache_bin_finish_flush(bin, info, &arr, nflush); cache_bin_finish_flush(bin, info, &arr, nflush);
expect_true(cache_bin_ncached_get(bin, info) == nfill - nflush, ""); expect_true(cache_bin_ncached_get_local(bin, info) == nfill - nflush,
while (cache_bin_ncached_get(bin, info) > 0) { "");
while (cache_bin_ncached_get_local(bin, info) > 0) {
cache_bin_alloc(bin, &success); cache_bin_alloc(bin, &success);
} }
} }
@ -55,14 +57,14 @@ do_flush_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
static void static void
do_batch_alloc_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs, do_batch_alloc_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
cache_bin_sz_t nfill, size_t batch) { cache_bin_sz_t nfill, size_t batch) {
assert_true(cache_bin_ncached_get(bin, info) == 0, ""); assert_true(cache_bin_ncached_get_local(bin, info) == 0, "");
CACHE_BIN_PTR_ARRAY_DECLARE(arr, nfill); CACHE_BIN_PTR_ARRAY_DECLARE(arr, nfill);
cache_bin_init_ptr_array_for_fill(bin, info, &arr, nfill); cache_bin_init_ptr_array_for_fill(bin, info, &arr, nfill);
for (cache_bin_sz_t i = 0; i < nfill; i++) { for (cache_bin_sz_t i = 0; i < nfill; i++) {
arr.ptr[i] = &ptrs[i]; arr.ptr[i] = &ptrs[i];
} }
cache_bin_finish_fill(bin, info, &arr, nfill); cache_bin_finish_fill(bin, info, &arr, nfill);
assert_true(cache_bin_ncached_get(bin, info) == nfill, ""); assert_true(cache_bin_ncached_get_local(bin, info) == nfill, "");
cache_bin_low_water_set(bin); cache_bin_low_water_set(bin);
void **out = malloc((batch + 1) * sizeof(void *)); void **out = malloc((batch + 1) * sizeof(void *));
@ -73,7 +75,7 @@ do_batch_alloc_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs,
} }
expect_true(cache_bin_low_water_get(bin, info) == nfill - expect_true(cache_bin_low_water_get(bin, info) == nfill -
(cache_bin_sz_t)n, ""); (cache_bin_sz_t)n, "");
while (cache_bin_ncached_get(bin, info) > 0) { while (cache_bin_ncached_get_local(bin, info) > 0) {
bool success; bool success;
cache_bin_alloc(bin, &success); cache_bin_alloc(bin, &success);
} }
@ -104,7 +106,7 @@ TEST_BEGIN(test_cache_bin) {
/* Initialize to empty; should then have 0 elements. */ /* Initialize to empty; should then have 0 elements. */
expect_d_eq(ncached_max, cache_bin_info_ncached_max(&info), ""); expect_d_eq(ncached_max, cache_bin_info_ncached_max(&info), "");
expect_true(cache_bin_ncached_get(&bin, &info) == 0, ""); expect_true(cache_bin_ncached_get_local(&bin, &info) == 0, "");
expect_true(cache_bin_low_water_get(&bin, &info) == 0, ""); expect_true(cache_bin_low_water_get(&bin, &info) == 0, "");
ptr = cache_bin_alloc_easy(&bin, &success); ptr = cache_bin_alloc_easy(&bin, &success);
@ -122,14 +124,15 @@ TEST_BEGIN(test_cache_bin) {
void **ptrs = mallocx(sizeof(void *) * (ncached_max + 1), 0); void **ptrs = mallocx(sizeof(void *) * (ncached_max + 1), 0);
assert_ptr_not_null(ptrs, "Unexpected mallocx failure"); assert_ptr_not_null(ptrs, "Unexpected mallocx failure");
for (cache_bin_sz_t i = 0; i < ncached_max; i++) { for (cache_bin_sz_t i = 0; i < ncached_max; i++) {
expect_true(cache_bin_ncached_get(&bin, &info) == i, ""); expect_true(cache_bin_ncached_get_local(&bin, &info) == i, "");
success = cache_bin_dalloc_easy(&bin, &ptrs[i]); success = cache_bin_dalloc_easy(&bin, &ptrs[i]);
expect_true(success, expect_true(success,
"Should be able to dalloc into a non-full cache bin."); "Should be able to dalloc into a non-full cache bin.");
expect_true(cache_bin_low_water_get(&bin, &info) == 0, expect_true(cache_bin_low_water_get(&bin, &info) == 0,
"Pushes and pops shouldn't change low water of zero."); "Pushes and pops shouldn't change low water of zero.");
} }
expect_true(cache_bin_ncached_get(&bin, &info) == ncached_max, ""); expect_true(cache_bin_ncached_get_local(&bin, &info) == ncached_max,
"");
success = cache_bin_dalloc_easy(&bin, &ptrs[ncached_max]); success = cache_bin_dalloc_easy(&bin, &ptrs[ncached_max]);
expect_false(success, "Shouldn't be able to dalloc into a full bin."); expect_false(success, "Shouldn't be able to dalloc into a full bin.");
@ -138,7 +141,7 @@ TEST_BEGIN(test_cache_bin) {
for (cache_bin_sz_t i = 0; i < ncached_max; i++) { for (cache_bin_sz_t i = 0; i < ncached_max; i++) {
expect_true(cache_bin_low_water_get(&bin, &info) expect_true(cache_bin_low_water_get(&bin, &info)
== ncached_max - i, ""); == ncached_max - i, "");
expect_true(cache_bin_ncached_get(&bin, &info) expect_true(cache_bin_ncached_get_local(&bin, &info)
== ncached_max - i, ""); == ncached_max - i, "");
/* /*
* This should fail -- the easy variant can't change the low * This should fail -- the easy variant can't change the low
@ -149,7 +152,7 @@ TEST_BEGIN(test_cache_bin) {
expect_false(success, ""); expect_false(success, "");
expect_true(cache_bin_low_water_get(&bin, &info) expect_true(cache_bin_low_water_get(&bin, &info)
== ncached_max - i, ""); == ncached_max - i, "");
expect_true(cache_bin_ncached_get(&bin, &info) expect_true(cache_bin_ncached_get_local(&bin, &info)
== ncached_max - i, ""); == ncached_max - i, "");
/* This should succeed, though. */ /* This should succeed, though. */
@ -159,11 +162,11 @@ TEST_BEGIN(test_cache_bin) {
"Alloc should pop in stack order"); "Alloc should pop in stack order");
expect_true(cache_bin_low_water_get(&bin, &info) expect_true(cache_bin_low_water_get(&bin, &info)
== ncached_max - i - 1, ""); == ncached_max - i - 1, "");
expect_true(cache_bin_ncached_get(&bin, &info) expect_true(cache_bin_ncached_get_local(&bin, &info)
== ncached_max - i - 1, ""); == ncached_max - i - 1, "");
} }
/* Now we're empty -- all alloc attempts should fail. */ /* Now we're empty -- all alloc attempts should fail. */
expect_true(cache_bin_ncached_get(&bin, &info) == 0, ""); expect_true(cache_bin_ncached_get_local(&bin, &info) == 0, "");
ptr = cache_bin_alloc_easy(&bin, &success); ptr = cache_bin_alloc_easy(&bin, &success);
expect_ptr_null(ptr, ""); expect_ptr_null(ptr, "");
expect_false(success, ""); expect_false(success, "");
@ -179,7 +182,8 @@ TEST_BEGIN(test_cache_bin) {
for (cache_bin_sz_t i = ncached_max / 2; i < ncached_max; i++) { for (cache_bin_sz_t i = ncached_max / 2; i < ncached_max; i++) {
cache_bin_dalloc_easy(&bin, &ptrs[i]); cache_bin_dalloc_easy(&bin, &ptrs[i]);
} }
expect_true(cache_bin_ncached_get(&bin, &info) == ncached_max, ""); expect_true(cache_bin_ncached_get_local(&bin, &info) == ncached_max,
"");
for (cache_bin_sz_t i = ncached_max - 1; i >= ncached_max / 2; i--) { for (cache_bin_sz_t i = ncached_max - 1; i >= ncached_max / 2; i--) {
/* /*
* Size is bigger than low water -- the reduced version should * Size is bigger than low water -- the reduced version should
@ -195,7 +199,7 @@ TEST_BEGIN(test_cache_bin) {
expect_ptr_null(ptr, ""); expect_ptr_null(ptr, "");
/* We're going to test filling -- we must be empty to start. */ /* We're going to test filling -- we must be empty to start. */
while (cache_bin_ncached_get(&bin, &info)) { while (cache_bin_ncached_get_local(&bin, &info)) {
cache_bin_alloc(&bin, &success); cache_bin_alloc(&bin, &success);
expect_true(success, ""); expect_true(success, "");
} }