From 8ca58cf82e430775ab1c2adfab31f77f649c3f96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20H=C3=B6gberg?= Date: Thu, 14 Jun 2018 08:26:07 +0200 Subject: Don't enqueue system tasks if target process is in fail_state The fail state wasn't re-checked in the state change loop; only the FREE state was checked. In addition to that, we would leave the task in the queue when bailing out which could lead to a double-free. This commit backports active_sys_enqueue from master to make it easier to merge onwards. --- erts/emulator/beam/erl_process.c | 228 ++++++++++++++++++++------------------- 1 file changed, 118 insertions(+), 110 deletions(-) (limited to 'erts/emulator/beam') diff --git a/erts/emulator/beam/erl_process.c b/erts/emulator/beam/erl_process.c index b0db2da243..dad4006beb 100644 --- a/erts/emulator/beam/erl_process.c +++ b/erts/emulator/beam/erl_process.c @@ -7156,129 +7156,72 @@ erts_schedule_process(Process *p, erts_aint32_t state, ErtsProcLocks locks) schedule_process(p, state, locks); } -static int -schedule_process_sys_task(Process *p, erts_aint32_t prio, ErtsProcSysTask *st, - erts_aint32_t *fail_state_p) -{ - int res; - int locked; - ErtsProcSysTaskQs *stqs, *free_stqs; - erts_aint32_t fail_state, state, a, n, enq_prio; +/* Enqueues the given sys task on the process and schedules it. The task may be + * NULL if only scheduling is desired. */ +static ERTS_INLINE erts_aint32_t +active_sys_enqueue(Process *p, ErtsProcSysTask *sys_task, + erts_aint32_t task_prio, erts_aint32_t enable_flags, + erts_aint32_t state, erts_aint32_t *fail_state_p) +{ + int runnable_procs = erts_system_profile_flags.runnable_procs; + erts_aint32_t n, a, enq_prio, fail_state; + int already_scheduled; + int status_locked; int enqueue; /* < 0 -> use proxy */ - unsigned int prof_runnable_procs; + enable_flags |= ERTS_PSFLG_ACTIVE_SYS; fail_state = *fail_state_p; - - res = 1; /* prepare for success */ - st->next = st->prev = st; /* Prep for empty prio queue */ - state = erts_smp_atomic32_read_nob(&p->state); - prof_runnable_procs = erts_system_profile_flags.runnable_procs; - locked = 0; - free_stqs = NULL; - if (state & ERTS_PSFLG_ACTIVE_SYS) - stqs = NULL; - else { - alloc_qs: - stqs = proc_sys_task_queues_alloc(); - stqs->qmask = 1 << prio; - stqs->ncount = 0; - stqs->q[PRIORITY_MAX] = NULL; - stqs->q[PRIORITY_HIGH] = NULL; - stqs->q[PRIORITY_NORMAL] = NULL; - stqs->q[PRIORITY_LOW] = NULL; - stqs->q[prio] = st; - } - - if (!locked) { - locked = 1; - erts_smp_proc_lock(p, ERTS_PROC_LOCK_STATUS); - - state = erts_smp_atomic32_read_nob(&p->state); - if (state & fail_state) { - *fail_state_p = (state & fail_state); - free_stqs = stqs; - res = 0; - goto cleanup; - } - } - - if (!p->sys_task_qs) { - if (stqs) - p->sys_task_qs = stqs; - else - goto alloc_qs; - } - else { - free_stqs = stqs; - stqs = p->sys_task_qs; - if (!stqs->q[prio]) { - stqs->q[prio] = st; - stqs->qmask |= 1 << prio; - } - else { - st->next = stqs->q[prio]; - st->prev = stqs->q[prio]->prev; - st->next->prev = st; - st->prev->next = st; - ASSERT(stqs->qmask & (1 << prio)); - } - } - - if (ERTS_PSFLGS_GET_ACT_PRIO(state) > prio) { - erts_aint32_t n, a, e; - /* Need to elevate actual prio */ - - a = state; - do { - if (ERTS_PSFLGS_GET_ACT_PRIO(a) <= prio) { - n = a; - break; - } - n = e = a; - n &= ~ERTS_PSFLGS_ACT_PRIO_MASK; - n |= (prio << ERTS_PSFLGS_ACT_PRIO_OFFSET); - a = erts_smp_atomic32_cmpxchg_nob(&p->state, n, e); - } while (a != e); - state = n; - } - - - a = state; + already_scheduled = 0; + status_locked = 0; enq_prio = -1; + a = state; - /* Status lock prevents out of order "runnable proc" trace msgs */ - ERTS_SMP_LC_ASSERT(ERTS_PROC_LOCK_STATUS & erts_proc_lc_my_proc_locks(p)); + ERTS_SMP_LC_ASSERT(!(ERTS_PROC_LOCK_STATUS & erts_proc_lc_my_proc_locks(p))); + ASSERT(fail_state & (ERTS_PSFLG_EXITING | ERTS_PSFLG_FREE)); + ASSERT(!(fail_state & enable_flags)); + ASSERT(!(state & ERTS_PSFLG_PROXY)); - if (!prof_runnable_procs) { - erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS); - locked = 0; + /* When runnable_procs is enabled, we need to take the status lock to + * prevent trace messages from being sent in the wrong order. The lock must + * be held over the call to add2runq. + * + * Otherwise, we only need to take it when we're enqueuing a task and can + * safely release it before add2runq. */ + if (sys_task || runnable_procs) { + erts_smp_proc_lock(p, ERTS_PROC_LOCK_STATUS); + status_locked = 1; } - ASSERT(!(state & ERTS_PSFLG_PROXY)); - while (1) { erts_aint32_t e; n = e = a; - if (a & ERTS_PSFLG_FREE) - goto cleanup; /* We don't want to schedule free processes... */ + if (a & fail_state) { + *fail_state_p = a & fail_state; + goto cleanup; + } enqueue = ERTS_ENQUEUE_NOT; - n |= ERTS_PSFLG_ACTIVE_SYS; + n |= enable_flags; + if (!(a & (ERTS_PSFLG_RUNNING | ERTS_PSFLG_RUNNING_SYS | ERTS_PSFLG_DIRTY_RUNNING - | ERTS_PSFLG_DIRTY_RUNNING_SYS))) + | ERTS_PSFLG_DIRTY_RUNNING_SYS))) { enqueue = check_enqueue_in_prio_queue(p, &enq_prio, &n, a); + } + a = erts_smp_atomic32_cmpxchg_mb(&p->state, n, e); - if (a == e) + if (a == e) { break; - if (a == n && enqueue == ERTS_ENQUEUE_NOT) - goto cleanup; + } + else if (a == n && enqueue == ERTS_ENQUEUE_NOT) { + already_scheduled = 1; + break; + } } - if (prof_runnable_procs) { - + if (!already_scheduled && runnable_procs) { if (!(a & (ERTS_PSFLG_ACTIVE_SYS | ERTS_PSFLG_RUNNING | ERTS_PSFLG_RUNNING_SYS @@ -7288,24 +7231,89 @@ schedule_process_sys_task(Process *p, erts_aint32_t prio, ErtsProcSysTask *st, /* We activated a prevously inactive process */ profile_runnable_proc(p, am_active); } + } - erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS); - locked = 0; + if (sys_task) { + ErtsProcSysTaskQs *stqs = p->sys_task_qs; + + if (!stqs) { + sys_task->next = sys_task->prev = sys_task; + + stqs = proc_sys_task_queues_alloc(); + + stqs->qmask = 1 << task_prio; + stqs->ncount = 0; + stqs->q[PRIORITY_MAX] = NULL; + stqs->q[PRIORITY_HIGH] = NULL; + stqs->q[PRIORITY_NORMAL] = NULL; + stqs->q[PRIORITY_LOW] = NULL; + stqs->q[task_prio] = sys_task; + + p->sys_task_qs = stqs; + } + else { + if (!stqs->q[task_prio]) { + sys_task->next = sys_task->prev = sys_task; + + stqs->q[task_prio] = sys_task; + stqs->qmask |= 1 << task_prio; + } + else { + sys_task->next = stqs->q[task_prio]; + sys_task->prev = stqs->q[task_prio]->prev; + sys_task->next->prev = sys_task; + sys_task->prev->next = sys_task; + ASSERT(stqs->qmask & (1 << task_prio)); + } + } + } + + if (status_locked && !runnable_procs) { + erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS); + status_locked = 0; } - add2runq(enqueue, enq_prio, p, n, NULL); + if (!already_scheduled) { + add2runq(enqueue, enq_prio, p, n, NULL); + } cleanup: + if (status_locked) { + erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS); + } - if (locked) - erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS); + return n; +} + +static int +schedule_process_sys_task(Process *p, erts_aint32_t prio, ErtsProcSysTask *st, + erts_aint32_t *fail_state_p) +{ + erts_aint32_t fail_state, state; - if (free_stqs) - proc_sys_task_queues_free(free_stqs); + /* Elevate priority if needed. */ + state = erts_smp_atomic32_read_nob(&p->state); + if (ERTS_PSFLGS_GET_ACT_PRIO(state) > prio) { + erts_aint32_t n, a, e; - ERTS_SMP_LC_ASSERT(!(ERTS_PROC_LOCK_STATUS & erts_proc_lc_my_proc_locks(p))); + a = state; + do { + if (ERTS_PSFLGS_GET_ACT_PRIO(a) <= prio) { + n = a; + break; + } + n = e = a; + n &= ~ERTS_PSFLGS_ACT_PRIO_MASK; + n |= (prio << ERTS_PSFLGS_ACT_PRIO_OFFSET); + a = erts_smp_atomic32_cmpxchg_nob(&p->state, n, e); + } while (a != e); - return res; + state = n; + } + + fail_state = *fail_state_p; + + return !(active_sys_enqueue(p, st, prio, 0, state, fail_state_p) & fail_state); } static ERTS_INLINE int -- cgit v1.2.3