From a011c4c22d3fd1da5415dd5001afd195f5cd7ad5 Mon Sep 17 00:00:00 2001 From: David Goldblatt Date: Thu, 7 Jan 2021 13:22:08 -0800 Subject: [PATCH] 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). --- include/jemalloc/internal/cache_bin.h | 54 +++++++++++++++++++++------ src/arena.c | 4 +- src/cache_bin.c | 2 +- src/tcache.c | 6 +-- test/unit/cache_bin.c | 40 +++++++++++--------- 5 files changed, 70 insertions(+), 36 deletions(-) diff --git a/include/jemalloc/internal/cache_bin.h b/include/jemalloc/internal/cache_bin.h index c1b8fc42..cf5ed3e0 100644 --- a/include/jemalloc/internal/cache_bin.h +++ b/include/jemalloc/internal/cache_bin.h @@ -167,20 +167,50 @@ cache_bin_diff(cache_bin_t *bin, uint16_t earlier, uint16_t later) { 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 -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, (uint16_t)(uintptr_t)bin->stack_head, bin->low_bits_empty); 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; } -/* 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 -cache_bin_ncached_get(cache_bin_t *bin, cache_bin_info_t *info) { - cache_bin_sz_t n = cache_bin_ncached_get_internal(bin); +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, + /* 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)); return n; } @@ -208,7 +238,7 @@ cache_bin_empty_position_get(cache_bin_t *bin) { */ static inline void 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); } @@ -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_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_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, bin->low_bits_low_water); @@ -247,7 +277,7 @@ cache_bin_low_water_set(cache_bin_t *bin) { static inline void 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_set(bin); } @@ -319,7 +349,7 @@ cache_bin_alloc(cache_bin_t *bin, bool *success) { JEMALLOC_ALWAYS_INLINE cache_bin_sz_t 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) { 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_ptr_array_t *arr, cache_bin_sz_t nflush) { 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); } @@ -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 cache_bin_finish_flush(cache_bin_t *bin, cache_bin_info_t *info, 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, rem * sizeof(void *)); bin->stack_head = bin->stack_head + nflushed; diff --git a/src/arena.c b/src/arena.c index 6a062de2..914e63f1 100644 --- a/src/arena.c +++ b/src/arena.c @@ -150,7 +150,7 @@ arena_stats_merge(tsdn_t *tsdn, arena_t *arena, unsigned *nthreads, for (szind_t i = 0; i < nhbins; i++) { cache_bin_t *cache_bin = &descriptor->bins[i]; astats->tcache_bytes += - cache_bin_ncached_get(cache_bin, + cache_bin_ncached_get_remote(cache_bin, &tcache_bin_info[i]) * sz_index2size(i); } } @@ -767,7 +767,7 @@ void 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, 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]; diff --git a/src/cache_bin.c b/src/cache_bin.c index 5f506062..b7470823 100644 --- a/src/cache_bin.c +++ b/src/cache_bin.c @@ -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; assert(cache_bin_diff(bin, bin->low_bits_full, (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(bin_stack_size > 0 || empty_position == full_position); diff --git a/src/tcache.c b/src/tcache.c index 41a1b828..ef0b87d0 100644 --- a/src/tcache.c +++ b/src/tcache.c @@ -125,7 +125,7 @@ tcache_gc_small(tsd_t *tsd, tcache_slow_t *tcache_slow, tcache_t *tcache, assert(szind < SC_NBINS); 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]); cache_bin_sz_t low_water = cache_bin_low_water_get(cache_bin, &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. */ assert(szind >= SC_NBINS); 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]); cache_bin_sz_t low_water = cache_bin_low_water_get(cache_bin, &tcache_bin_info[szind]); @@ -289,7 +289,7 @@ tcache_bin_flush_impl(tsd_t *tsd, tcache_t *tcache, cache_bin_t *cache_bin, } else { 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]); assert((cache_bin_sz_t)rem <= ncached); arena_t *tcache_arena = tcache_slow->arena; diff --git a/test/unit/cache_bin.c b/test/unit/cache_bin.c index b31d07d2..a69cad6b 100644 --- a/test/unit/cache_bin.c +++ b/test/unit/cache_bin.c @@ -6,14 +6,15 @@ do_fill_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs, cache_bin_sz_t nfill_succeed) { bool success; 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_init_ptr_array_for_fill(bin, info, &arr, nfill_attempt); for (cache_bin_sz_t i = 0; i < nfill_succeed; i++) { arr.ptr[i] = &ptrs[i]; } 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); 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) == 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, ""); } @@ -32,7 +33,7 @@ static void do_flush_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs, cache_bin_sz_t nfill, cache_bin_sz_t nflush) { 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++) { 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); - expect_true(cache_bin_ncached_get(bin, info) == nfill - nflush, ""); - while (cache_bin_ncached_get(bin, info) > 0) { + expect_true(cache_bin_ncached_get_local(bin, info) == nfill - nflush, + ""); + while (cache_bin_ncached_get_local(bin, info) > 0) { 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 do_batch_alloc_test(cache_bin_t *bin, cache_bin_info_t *info, void **ptrs, 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_init_ptr_array_for_fill(bin, info, &arr, nfill); for (cache_bin_sz_t i = 0; i < nfill; i++) { arr.ptr[i] = &ptrs[i]; } 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); 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 - (cache_bin_sz_t)n, ""); - while (cache_bin_ncached_get(bin, info) > 0) { + while (cache_bin_ncached_get_local(bin, info) > 0) { bool success; cache_bin_alloc(bin, &success); } @@ -104,7 +106,7 @@ TEST_BEGIN(test_cache_bin) { /* Initialize to empty; should then have 0 elements. */ 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, ""); 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); assert_ptr_not_null(ptrs, "Unexpected mallocx failure"); 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]); expect_true(success, "Should be able to dalloc into a non-full cache bin."); expect_true(cache_bin_low_water_get(&bin, &info) == 0, "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]); 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++) { expect_true(cache_bin_low_water_get(&bin, &info) == ncached_max - i, ""); - expect_true(cache_bin_ncached_get(&bin, &info) + expect_true(cache_bin_ncached_get_local(&bin, &info) == ncached_max - i, ""); /* * This should fail -- the easy variant can't change the low @@ -149,7 +152,7 @@ TEST_BEGIN(test_cache_bin) { expect_false(success, ""); expect_true(cache_bin_low_water_get(&bin, &info) == ncached_max - i, ""); - expect_true(cache_bin_ncached_get(&bin, &info) + expect_true(cache_bin_ncached_get_local(&bin, &info) == ncached_max - i, ""); /* This should succeed, though. */ @@ -159,11 +162,11 @@ TEST_BEGIN(test_cache_bin) { "Alloc should pop in stack order"); expect_true(cache_bin_low_water_get(&bin, &info) == 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, ""); } /* 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); expect_ptr_null(ptr, ""); 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++) { 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--) { /* * Size is bigger than low water -- the reduced version should @@ -195,7 +199,7 @@ TEST_BEGIN(test_cache_bin) { expect_ptr_null(ptr, ""); /* 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); expect_true(success, ""); }