From b016246096ff8eb67d522a29d2279bead2d29923 Mon Sep 17 00:00:00 2001 From: Matthew Flatt Date: Fri, 28 Aug 2015 16:38:05 -0600 Subject: [PATCH] avoid interferences among places for memory accounting All places uses the same accounting bit for objects that are in the shared space. Each place also flips the bit value it wants on each accounting, so if two places are accounting at the same time with opposite bit values and can reach the same objects, they can interefere. It's even possible for them to race through cycles and cause each other to loop forever. Add a lock to ensure that there's only one bit value in play for the shared space at any given time. A place must stall if other places are busy with memory accounting and an opposite bit value. --- .../tests/racket/place-chmsg-gc-acct.rkt | 1 - racket/src/racket/gc2/mem_account.c | 81 ++++++++++++++++++- racket/src/racket/gc2/newgc.c | 31 +++---- racket/src/racket/gc2/newgc.h | 1 + 4 files changed, 95 insertions(+), 19 deletions(-) diff --git a/pkgs/racket-test/tests/racket/place-chmsg-gc-acct.rkt b/pkgs/racket-test/tests/racket/place-chmsg-gc-acct.rkt index 1bcb74dce5..b03e788d2a 100644 --- a/pkgs/racket-test/tests/racket/place-chmsg-gc-acct.rkt +++ b/pkgs/racket-test/tests/racket/place-chmsg-gc-acct.rkt @@ -6,7 +6,6 @@ pch ;; Trigger memory accounting in every place: - #; (custodian-limit-memory (current-custodian) (* 1024 1024 1024)) diff --git a/racket/src/racket/gc2/mem_account.c b/racket/src/racket/gc2/mem_account.c index abf104b406..dee0990d92 100644 --- a/racket/src/racket/gc2/mem_account.c +++ b/racket/src/racket/gc2/mem_account.c @@ -268,15 +268,85 @@ inline static uintptr_t custodian_usage(NewGC*gc, void *custodian) return gcWORDS_TO_BYTES(retval); } +#ifdef MZ_USE_PLACES + +static mzrt_mutex *master_btc_lock; +static mzrt_sema *master_btc_sema; +static int master_btc_lock_count = 0; +static int master_btc_lock_waiters = 0; + +void init_master_btc_locks() +{ + mzrt_mutex_create(&master_btc_lock); + mzrt_sema_create(&master_btc_sema, 0); +} + +static void check_master_btc_mark(NewGC *gc, mpage *page) +{ + if (!gc->master_page_btc_mark_checked) { + int pause = 1; + RELEASE_PAGE_LOCK(1, page); + while (pause) { + mzrt_mutex_lock(master_btc_lock); + if (master_btc_lock_count + && (gc->new_btc_mark != MASTERGC->new_btc_mark)) { + pause = 1; + master_btc_lock_waiters++; + } else { + pause = 0; + MASTERGC->new_btc_mark = gc->new_btc_mark; + master_btc_lock_count++; + } + mzrt_mutex_unlock(master_btc_lock); + + if (pause) + mzrt_sema_wait(master_btc_sema); + } + TAKE_PAGE_LOCK(1, page); + gc->master_page_btc_mark_checked = 1; + } +} + +static void release_master_btc_mark(NewGC *gc) +{ + if (gc->master_page_btc_mark_checked) { + /* release the lock on the master's new_btc_mark value */ + mzrt_mutex_lock(master_btc_lock); + --master_btc_lock_count; + if (!master_btc_lock_count && master_btc_lock_waiters) { + --master_btc_lock_waiters; + mzrt_sema_post(master_btc_sema); + } + mzrt_mutex_unlock(master_btc_lock); + } +} + +#else + +static void check_master_btc_mark(NewGC *gc) { } +static void release_master_btc_mark(NewGC *gc) { } + +#endif + inline static void BTC_memory_account_mark(NewGC *gc, mpage *page, void *ptr, int is_a_master_page) { GCDEBUG((DEBUGOUTF, "BTC_memory_account_mark: %p/%p\n", page, ptr)); /* In the case of is_a_master_page, whether this place is charged is - a little random: there's no guarantee that the btc_mark values are - in sync, and there are races among places. Approximations are ok for - accounting, though, as long as the probably for completely wrong - accounting is very low. */ + a little random: there's no guarantee that the btc_mark values + are in sync, and there are races among places. Approximations are + ok for accounting, though, as long as the probably for completely + wrong accounting is very low. + + At the same time, we need to synchronize enough so that two + places with different new_btc_mark values don't send each other + into infinite loops (with the btc_mark value bouncing back and + forth) or overcounting. We synchronize enough by having a single + new_btc_mark value for master pages, and we stall if the value + isn't what this place wants. */ + + if (is_a_master_page) + check_master_btc_mark(gc, page); if(page->size_class) { if(page->size_class > 1) { @@ -427,6 +497,7 @@ static void BTC_do_accounting(NewGC *gc) gc->doing_memory_accounting = 1; gc->in_unsafe_allocation_mode = 1; gc->unsafe_allocation_abort = btc_overmem_abort; + gc->master_page_btc_mark_checked = 0; /* clear the memory use numbers out */ for(i = 1; i < table_size; i++) @@ -468,6 +539,8 @@ static void BTC_do_accounting(NewGC *gc) gc->phantom_count = save_count; } + release_master_btc_mark(gc); + /* walk backward folding totals int parent */ cur = last; while (cur) { diff --git a/racket/src/racket/gc2/newgc.c b/racket/src/racket/gc2/newgc.c index 9ac503c3e0..ca08b0d627 100644 --- a/racket/src/racket/gc2/newgc.c +++ b/racket/src/racket/gc2/newgc.c @@ -178,6 +178,20 @@ struct Log_Master_Info { # define PLACES_AND(v) 0 #endif +#ifdef MZ_USE_PLACES +static void adjust_page_lock(int is_a_master_page, mpage *page, intptr_t prev, intptr_t next) +{ + if (is_a_master_page) { + while (!mzrt_cas(&page->page_lock, prev, next)) { /* spin! */ } + } +} +# define TAKE_PAGE_LOCK(is_a_master_page, page) adjust_page_lock(is_a_master_page, page, 0, 1); +# define RELEASE_PAGE_LOCK(is_a_master_page, page) adjust_page_lock(is_a_master_page, page, 1, 0); +#else +# define TAKE_PAGE_LOCK(is_a_master_page, page) /* empty */ +# define RELEASE_PAGE_LOCK(is_a_master_page, page) /* empty */ +#endif + inline static size_t real_page_size(mpage* page); inline static int page_mmu_type(mpage *page); inline static int page_mmu_protectable(mpage *page); @@ -3137,6 +3151,9 @@ void GC_init_type_tags(int count, int pair, int mutable_pair, int weakbox, int e initialized = 1; init_type_tags_worker(NULL, NULL, count, pair, mutable_pair, weakbox, ephemeron, weakarray, custbox, phantom); +#if defined(MZ_USE_PLACES) && defined(NEWGC_BTC_ACCOUNT) + init_master_btc_locks(); +#endif } else { GCPRINT(GCOUTF, "GC_init_type_tags should only be called once!\n"); abort(); @@ -3406,20 +3423,6 @@ static void mark_recur_or_push_ptr(struct NewGC *gc, void *p, int is_a_master_pa push_ptr(gc, p); } -#ifdef MZ_USE_PLACES -static void adjust_page_lock(int is_a_master_page, mpage *page, intptr_t prev, intptr_t next) -{ - if (is_a_master_page) { - while (!mzrt_cas(&page->page_lock, prev, next)) { /* spin! */ } - } -} -# define TAKE_PAGE_LOCK(is_a_master_page, page) adjust_page_lock(is_a_master_page, page, 0, 1); -# define RELEASE_PAGE_LOCK(is_a_master_page, page) adjust_page_lock(is_a_master_page, page, 1, 0); -#else -# define TAKE_PAGE_LOCK(is_a_master_page, page) /* empty */ -# define RELEASE_PAGE_LOCK(is_a_master_page, page) /* empty */ -#endif - /* This is the first mark routine. It's a bit complicated. */ void GC_mark2(const void *const_p, struct NewGC *gc) { diff --git a/racket/src/racket/gc2/newgc.h b/racket/src/racket/gc2/newgc.h index e73cbb09b3..677a8927b8 100644 --- a/racket/src/racket/gc2/newgc.h +++ b/racket/src/racket/gc2/newgc.h @@ -184,6 +184,7 @@ typedef struct NewGC { OTEntry **owner_table; unsigned int owner_table_size; AccountHook *hooks; + int master_page_btc_mark_checked; uintptr_t number_of_gc_runs; unsigned int since_last_full;