From 8918461fc41576664719c12ce7d655aaad2c78d1 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Fri, 17 May 2013 17:46:01 -0700 Subject: [PATCH] rt: Release big stacks immediately after use to avoid holding on to them through yields This avoids the following pathological scenario that makes threadring OOM: 1) task calls C using fast_ffi, borrowing a big stack from the scheduler. 2) task returns from C and places the big stack on the task-local stack segment list 3) task calls further Rust functions that require growing the stack, and for this reuses the big stack 4) task yields, failing to return the big stack to the scheduler. 5) repeat 500+ times and OOM Conflicts: src/rt/rust_task.cpp --- src/rt/rust_sched_loop.h | 3 +- src/rt/rust_task.cpp | 56 +++++++-------------------- src/rt/rust_task.h | 19 +++++++-- src/test/bench/shootout-threadring.rs | 18 ++++----- 4 files changed, 40 insertions(+), 56 deletions(-) diff --git a/src/rt/rust_sched_loop.h b/src/rt/rust_sched_loop.h index a099c5e0c74..e0101c46fb9 100644 --- a/src/rt/rust_sched_loop.h +++ b/src/rt/rust_sched_loop.h @@ -211,12 +211,13 @@ rust_sched_loop::return_c_stack(stk_seg *stack) { // NB: Runs on the Rust stack. Might return NULL! inline stk_seg * rust_sched_loop::borrow_big_stack() { - assert(cached_big_stack); stk_seg *your_stack; if (extra_big_stack) { your_stack = extra_big_stack; extra_big_stack = NULL; } else { + // NB: This may be null if we're asking for a *second* + // big stack, in which case the caller will fall back to a slow path your_stack = cached_big_stack; cached_big_stack = NULL; } diff --git a/src/rt/rust_task.cpp b/src/rt/rust_task.cpp index 81ae991623f..4d6d2567cd4 100644 --- a/src/rt/rust_task.cpp +++ b/src/rt/rust_task.cpp @@ -54,8 +54,7 @@ rust_task::rust_task(rust_sched_loop *sched_loop, rust_task_state state, disallow_yield(0), c_stack(NULL), next_c_sp(0), - next_rust_sp(0), - big_stack(NULL) + next_rust_sp(0) { LOGPTR(sched_loop, "new task", (uintptr_t)this); DLOG(sched_loop, task, "sizeof(task) = %d (0x%x)", @@ -566,14 +565,8 @@ rust_task::cleanup_after_turn() { while (stk->next) { stk_seg *new_next = stk->next->next; - - if (stk->next->is_big) { - assert (big_stack == stk->next); - sched_loop->return_big_stack(big_stack); - big_stack = NULL; - } else { - free_stack(stk->next); - } + assert (!stk->next->is_big); + free_stack(stk->next); stk->next = new_next; } @@ -584,38 +577,20 @@ rust_task::cleanup_after_turn() { bool rust_task::new_big_stack() { assert(stk); - // If we have a cached big stack segment, use it. - if (big_stack) { - // Check to see if we're already on the big stack. - stk_seg *ss = stk; - while (ss != NULL) { - if (ss == big_stack) - return false; - ss = ss->prev; - } - // Unlink the big stack. - if (big_stack->next) - big_stack->next->prev = big_stack->prev; - if (big_stack->prev) - big_stack->prev->next = big_stack->next; - } else { - stk_seg *borrowed_big_stack = sched_loop->borrow_big_stack(); - if (!borrowed_big_stack) { - abort(); - } else { - big_stack = borrowed_big_stack; - } + stk_seg *borrowed_big_stack = sched_loop->borrow_big_stack(); + if (!borrowed_big_stack) { + return false; } - big_stack->task = this; - big_stack->next = stk->next; - if (big_stack->next) - big_stack->next->prev = big_stack; - big_stack->prev = stk; - stk->next = big_stack; + borrowed_big_stack->task = this; + borrowed_big_stack->next = stk->next; + if (borrowed_big_stack->next) + borrowed_big_stack->next->prev = borrowed_big_stack; + borrowed_big_stack->prev = stk; + stk->next = borrowed_big_stack; - stk = big_stack; + stk = borrowed_big_stack; return true; } @@ -640,10 +615,9 @@ void rust_task::reset_stack_limit() { uintptr_t sp = get_sp(); while (!sp_in_stk_seg(sp, stk)) { - stk = stk->prev; + prev_stack(); assert(stk != NULL && "Failed to find the current stack"); } - record_stack_limit(); } void @@ -667,8 +641,6 @@ rust_task::delete_all_stacks() { stk = prev; } - - big_stack = NULL; } /* diff --git a/src/rt/rust_task.h b/src/rt/rust_task.h index 982102cdde0..1735d35b065 100644 --- a/src/rt/rust_task.h +++ b/src/rt/rust_task.h @@ -278,9 +278,6 @@ private: uintptr_t next_c_sp; uintptr_t next_rust_sp; - // The big stack. - stk_seg *big_stack; - // Called when the atomic refcount reaches zero void delete_this(); @@ -607,7 +604,21 @@ rust_task::prev_stack() { // require switching to the C stack and be costly. Instead we'll just move // up the link list and clean up later, either in new_stack or after our // turn ends on the scheduler. - stk = stk->prev; + if (stk->is_big) { + stk_seg *ss = stk; + stk = stk->prev; + + // Unlink the big stack. + if (ss->next) + ss->next->prev = ss->prev; + if (ss->prev) + ss->prev->next = ss->next; + + sched_loop->return_big_stack(ss); + } else { + stk = stk->prev; + } + record_stack_limit(); } diff --git a/src/test/bench/shootout-threadring.rs b/src/test/bench/shootout-threadring.rs index 213c5140ccf..a67bbb05dfb 100644 --- a/src/test/bench/shootout-threadring.rs +++ b/src/test/bench/shootout-threadring.rs @@ -10,18 +10,18 @@ // Based on threadring.erlang by Jira Isa -// xfail-test FIXME #5985 OOM's on the mac bot +use std::os; fn start(n_tasks: int, token: int) { - let mut (p, ch1) = comm::stream(); + let mut (p, ch1) = stream(); ch1.send(token); // XXX could not get this to work with a range closure let mut i = 2; while i <= n_tasks { - let (next_p, ch) = comm::stream(); + let (next_p, ch) = stream(); let imm_i = i; let imm_p = p; - do task::spawn { + do spawn { roundtrip(imm_i, n_tasks, &imm_p, &ch); }; p = next_p; @@ -29,16 +29,16 @@ fn start(n_tasks: int, token: int) { } let imm_p = p; let imm_ch = ch1; - do task::spawn { + do spawn { roundtrip(1, n_tasks, &imm_p, &imm_ch); } } -fn roundtrip(id: int, n_tasks: int, p: &comm::Port, ch: &comm::Chan) { +fn roundtrip(id: int, n_tasks: int, p: &Port, ch: &Chan) { while (true) { match p.recv() { 1 => { - io::println(fmt!("%d\n", id)); + println(fmt!("%d\n", id)); return; } token => { @@ -60,13 +60,13 @@ fn main() { os::args() }; let token = if args.len() > 1u { - int::from_str(args[1]).get() + FromStr::from_str(args[1]).get() } else { 1000 }; let n_tasks = if args.len() > 2u { - int::from_str(args[2]).get() + FromStr::from_str(args[2]).get() } else { 503