From 18c03b13d1a53282e69160fa71a7195a11e84392 Mon Sep 17 00:00:00 2001 From: Rickard Green Date: Thu, 7 Aug 2014 22:02:25 +0200 Subject: Fix emigrate bug in erts_port_task_schedule() While current run-queue lock is unlocked in the call to erts_check_emigration_need() from erts_port_task_schedule() the port can be migrated to another run-queue by another thread. The code in erts_port_task_schedule() needs to check if this has occurred when returning from erts_check_emigration_need(), and if so respect the migration decision. When this was not done, the thread calling erts_port_task_schedule() held the wrong run-queue lock which caused invalid updates of the port task queue. This bug was automatically fixed by the rewrites in the branch rickard/r16b/port-optimizations-fixes/OTP-10336 (commit 56cef897ca3ad2377e34a6ea5800a54a28cbeb6e) introduced in erts-5.10 and do not effect erts versions after that. --- erts/emulator/beam/erl_port_task.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) (limited to 'erts/emulator/beam') diff --git a/erts/emulator/beam/erl_port_task.c b/erts/emulator/beam/erl_port_task.c index 3dc7c14faf..11027dda7a 100644 --- a/erts/emulator/beam/erl_port_task.c +++ b/erts/emulator/beam/erl_port_task.c @@ -576,8 +576,20 @@ erts_port_task_schedule(Eterm id, if (!pp->sched.taskq && !pp->sched.exe_taskq) { #ifdef ERTS_SMP ErtsRunQueue *xrunq = erts_check_emigration_need(runq, ERTS_PORT_PRIO_LEVEL); - if (xrunq) { - /* Port emigrated ... */ + ERTS_SMP_LC_ASSERT(runq != xrunq); + if (runq != (ErtsRunQueue *) erts_smp_atomic_read_nob(&pp->run_queue)) { + /* + * Someone else migrated this port while we had the run-queue + * lock on runq unlocked in erts_check_emigration_need(). Respect + * that migration decision... + */ + erts_smp_runq_unlock(runq); + if (xrunq) + erts_smp_runq_unlock(xrunq); + runq = erts_port_runq(pp); + } + else if (xrunq) { + /* Emigrate port... */ erts_smp_atomic_set_nob(&pp->run_queue, (erts_aint_t) xrunq); erts_smp_runq_unlock(runq); runq = xrunq; @@ -929,6 +941,8 @@ erts_port_task_execute(ErtsRunQueue *runq, Port **curr_port_pp) #ifdef ERTS_SMP xrunq = erts_check_emigration_need(runq, ERTS_PORT_PRIO_LEVEL); + ERTS_SMP_LC_ASSERT(runq != xrunq); + ERTS_SMP_LC_ASSERT(runq == (ErtsRunQueue *) erts_smp_atomic_read_nob(&pp->run_queue)); if (!xrunq) { #endif enqueue_port(runq, pp); @@ -938,7 +952,7 @@ erts_port_task_execute(ErtsRunQueue *runq, Port **curr_port_pp) #ifdef ERTS_SMP } else { - /* Port emigrated ... */ + /* Emigrate port... */ erts_smp_atomic_set_nob(&pp->run_queue, (erts_aint_t) xrunq); enqueue_port(xrunq, pp); ASSERT(pp->sched.exe_taskq); -- cgit v1.2.3 From b6a5919cf78028c2778985e8ec6eccc467bb6039 Mon Sep 17 00:00:00 2001 From: Rickard Green Date: Thu, 14 Aug 2014 16:04:56 +0200 Subject: Verify run-queue asserts --- erts/emulator/beam/erl_port_task.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'erts/emulator/beam') diff --git a/erts/emulator/beam/erl_port_task.c b/erts/emulator/beam/erl_port_task.c index 11027dda7a..738f7d318b 100644 --- a/erts/emulator/beam/erl_port_task.c +++ b/erts/emulator/beam/erl_port_task.c @@ -76,6 +76,13 @@ do { \ #define DTRACE_DRIVER(PROBE_NAME, PP) do {} while(0) #endif +#define ERTS_SMP_LC_VERIFY_RQ(RQ, PP) \ + do { \ + ERTS_SMP_LC_ASSERT(erts_smp_lc_runq_is_locked(runq)); \ + ERTS_SMP_LC_ASSERT((RQ) == ((ErtsRunQueue *) \ + erts_smp_atomic_read_nob(&(PP)->run_queue))); \ + } while (0) + erts_smp_atomic_t erts_port_task_outstanding_io_tasks; struct ErtsPortTaskQueue_ { @@ -786,7 +793,9 @@ erts_port_task_execute(ErtsRunQueue *runq, Port **curr_port_pp) erts_smp_port_lock(pp); erts_smp_runq_lock(runq); } - + + ERTS_SMP_LC_VERIFY_RQ(runq, pp); + if (erts_sched_stat.enabled) { ErtsSchedulerData *esdp = erts_get_scheduler_data(); Uint old = ERTS_PORT_SCHED_ID(pp, esdp->no); @@ -828,6 +837,7 @@ erts_port_task_execute(ErtsRunQueue *runq, Port **curr_port_pp) case ERTS_PORT_TASK_FREE: /* May be pushed in q at any time */ reds += ERTS_PORT_REDS_FREE; erts_smp_runq_lock(runq); + ERTS_SMP_LC_VERIFY_RQ(runq, pp); erts_unblock_fpe(fpe_was_unmasked); ASSERT(pp->status & ERTS_PORT_SFLG_FREE_SCHEDULED); @@ -906,6 +916,7 @@ erts_port_task_execute(ErtsRunQueue *runq, Port **curr_port_pp) port_task_free(ptp); erts_smp_runq_lock(runq); + ERTS_SMP_LC_VERIFY_RQ(runq, pp); ptp = pop_task(ptqp); } @@ -942,7 +953,7 @@ erts_port_task_execute(ErtsRunQueue *runq, Port **curr_port_pp) #ifdef ERTS_SMP xrunq = erts_check_emigration_need(runq, ERTS_PORT_PRIO_LEVEL); ERTS_SMP_LC_ASSERT(runq != xrunq); - ERTS_SMP_LC_ASSERT(runq == (ErtsRunQueue *) erts_smp_atomic_read_nob(&pp->run_queue)); + ERTS_SMP_LC_VERIFY_RQ(runq, pp); if (!xrunq) { #endif enqueue_port(runq, pp); @@ -1052,6 +1063,7 @@ handle_remaining_tasks(ErtsRunQueue *runq, Port *pp) port_task_free(ptp); erts_smp_runq_lock(runq); + ERTS_SMP_LC_VERIFY_RQ(runq, pp); ptp = pop_task(ptqps[i]); } } -- cgit v1.2.3