erts: Only allow remove from trace_status callback

Make it so that it is only possible to remove a tracer via
returning remove from an erl_tracer. This limition is put in
place in order to avoid a lot of lock checking and taking
in various places, especially in regards to trace events
happening on dirty schedulers.

7 files changed, 85 insertions, 67 deletions

diff --git a/erts/emulator/beam/beam_bp.c b/erts/emulator/beam/beam_bp.c
index 308e5ce205..8489897d3a 100644
--- a/erts/emulator/beam/beam_bp.c
+++ b/erts/emulator/beam/beam_bp.c
@@ -655,8 +655,7 @@ erts_generic_breakpoint(Process* c_p, BeamInstr* I, Eterm* reg)
 	erts_smp_atomic_inc_nob(&bp->count->acount);
     }
 
-    if (bp_flags & ERTS_BPF_TIME_TRACE_ACTIVE
-        && ERTS_TRACER_PROC_IS_ENABLED(c_p)) {
+    if (bp_flags & ERTS_BPF_TIME_TRACE_ACTIVE) {
 	Eterm w;
 	erts_trace_time_call(c_p, I, bp->time);
 	w = (BeamInstr) *c_p->cp;
@@ -753,8 +752,7 @@ erts_bif_trace(int bif_index, Process* p, Eterm* args, BeamInstr* I)
 	}
     }
     if (bp_flags & ERTS_BPF_TIME_TRACE_ACTIVE &&
-	IS_TRACED_FL(p, F_TRACE_CALLS) &&
-	ERTS_TRACER_PROC_IS_ENABLED(p)) {
+	IS_TRACED_FL(p, F_TRACE_CALLS)) {
 	BeamInstr *pc = (BeamInstr *)ep->code+3;
 	erts_trace_time_call(p, pc, bp->time);
     }
diff --git a/erts/emulator/beam/erl_bif_trace.c b/erts/emulator/beam/erl_bif_trace.c
index b65c0e303f..66e5146da0 100644
--- a/erts/emulator/beam/erl_bif_trace.c
+++ b/erts/emulator/beam/erl_bif_trace.c
@@ -512,8 +512,7 @@ start_trace(Process *c_p, ErtsTracer tracer,
             && !ERTS_TRACER_COMPARE(ERTS_TRACER(port), tracer)) {
             /* This tracee is already being traced, and not by the
              * tracer to be */
-            if (erts_is_tracer_proc_enabled(c_p, ERTS_PROC_LOCKS_ALL,
-                                            common, am_trace_status)) {
+            if (erts_is_tracer_enabled(tracer, common)) {
                 /* The tracer is still in use */
                 return 1;
             }
@@ -856,7 +855,7 @@ trace_info_pid(Process* p, Eterm pid_spec, Eterm key)
             return am_undefined;
 
         if (!ERTS_TRACER_IS_NIL(ERTS_TRACER(tracee)))
-            erts_is_tracer_proc_enabled(NULL, 0, &tracee->common, am_trace_status);
+            erts_is_tracer_proc_enabled(NULL, 0, &tracee->common);
 
         tracer = erts_tracer_to_term(p, ERTS_TRACER(tracee));
         trace_flags = ERTS_TRACE_FLAGS(tracee);
@@ -864,22 +863,24 @@ trace_info_pid(Process* p, Eterm pid_spec, Eterm key)
         erts_port_release(tracee);
 
     } else if (is_internal_pid(pid_spec)) {
-	Process *tracee;
-	tracee = erts_pid2proc(p, ERTS_PROC_LOCK_MAIN,
-			       pid_spec, ERTS_PROC_LOCK_MAIN);
+	Process *tracee = erts_pid2proc_not_running(p, ERTS_PROC_LOCK_MAIN,
+                                                    pid_spec, ERTS_PROC_LOCK_MAIN);
+
+        if (tracee == ERTS_PROC_LOCK_BUSY)
+            ERTS_BIF_YIELD2(bif_export[BIF_trace_info_2], p, pid_spec, key);
 
 	if (!tracee)
 	    return am_undefined;
 
         if (!ERTS_TRACER_IS_NIL(ERTS_TRACER(tracee)))
             erts_is_tracer_proc_enabled(tracee, ERTS_PROC_LOCK_MAIN,
-                                        &tracee->common, am_trace_status);
+                                        &tracee->common);
 
         tracer = erts_tracer_to_term(p, ERTS_TRACER(tracee));
         trace_flags = ERTS_TRACE_FLAGS(tracee);
 
-        if (tracee != p)
-            erts_smp_proc_unlock(tracee, ERTS_PROC_LOCK_MAIN);
+	if (tracee != p)
+	    erts_smp_proc_unlock(tracee, ERTS_PROC_LOCK_MAIN);
     } else if (is_external_pid(pid_spec)
 	       && external_pid_dist_entry(pid_spec) == erts_this_dist_entry) {
 	    return am_undefined;
diff --git a/erts/emulator/beam/erl_db_util.c b/erts/emulator/beam/erl_db_util.c
index 3e75b9fd5f..6732b708a8 100644
--- a/erts/emulator/beam/erl_db_util.c
+++ b/erts/emulator/beam/erl_db_util.c
@@ -174,7 +174,8 @@ set_match_trace(Process *tracee_p, Eterm fail_term, ErtsTracer tracer,
         ERTS_PROC_LOCKS_ALL == erts_proc_lc_my_proc_locks(tracee_p)
         || erts_thr_progress_is_blocking());
 
-    if (ERTS_TRACER_IS_NIL(tracer) || erts_is_tracer_enabled(tracee_p, tracer))
+    if (ERTS_TRACER_IS_NIL(tracer)
+        || erts_is_tracer_enabled(tracer, &tracee_p->common))
         return set_tracee_flags(tracee_p, tracer, d_flags, e_flags);
     return fail_term;
 }
diff --git a/erts/emulator/beam/erl_process.c b/erts/emulator/beam/erl_process.c
index b50ca6d009..9d895f1867 100644
--- a/erts/emulator/beam/erl_process.c
+++ b/erts/emulator/beam/erl_process.c
@@ -10085,7 +10085,10 @@ Process *schedule(Process *p, int calls)
 
 	erts_smp_proc_unlock(p, ERTS_PROC_LOCK_STATUS);
 
-	if (IS_TRACED(p)) {
+        /* Clear tracer if it has been removed */
+	if (IS_TRACED(p) && erts_is_tracer_proc_enabled(
+                p, ERTS_PROC_LOCK_MAIN, &p->common)) {
+
 	    if (state & ERTS_PSFLG_EXITING) {
 		if (ARE_TRACE_FLAGS_ON(p, F_TRACE_SCHED_EXIT))
 		    trace_sched(p, ERTS_PROC_LOCK_MAIN, am_in_exiting);
@@ -10100,12 +10103,6 @@ Process *schedule(Process *p, int calls)
 	    }
 	}
 
-
-#ifdef ERTS_SMP
-        /* Clears tracer if it has been removed */
-        (void)ERTS_TRACER_PROC_IS_ENABLED(p);
-#endif
-
 	if (state & (ERTS_PSFLG_RUNNING_SYS
 		     | ERTS_PSFLG_DIRTY_RUNNING_SYS)) {
 	    /*
diff --git a/erts/emulator/beam/erl_trace.c b/erts/emulator/beam/erl_trace.c
index 1c0fc0a11f..447b46d239 100644
--- a/erts/emulator/beam/erl_trace.c
+++ b/erts/emulator/beam/erl_trace.c
@@ -396,7 +396,7 @@ send_to_tracer_nif(Process *c_p, ErtsPTabElementCommon *t_p,
                    Eterm tag, Eterm msg, Eterm extra,
                    Eterm pam_result);
 static ERTS_INLINE Eterm
-call_enabled_tracer(Process *c_p, const ErtsTracer tracer,
+call_enabled_tracer(const ErtsTracer tracer,
                     ErtsTracerNif **tnif_ref,
                     enum ErtsTracerOpt topt,
                     Eterm tag, Eterm t_p_id);
@@ -459,8 +459,7 @@ erts_set_system_seq_tracer(Process *c_p, ErtsProcLocks c_p_locks, ErtsTracer new
 
     if (!ERTS_TRACER_IS_NIL(new)) {
         Eterm nif_result = call_enabled_tracer(
-            NULL, new, NULL,
-            TRACE_FUN_ENABLED, am_trace_status, am_undefined);
+            new, NULL, TRACE_FUN_ENABLED, am_trace_status, am_undefined);
         switch (nif_result) {
         case am_trace: break;
         default:
@@ -492,7 +491,7 @@ erts_get_system_seq_tracer(void)
     erts_smp_rwmtx_runlock(&sys_trace_rwmtx);
 
     if (st != erts_tracer_nil &&
-        call_enabled_tracer(NULL, st, NULL, TRACE_FUN_ENABLED,
+        call_enabled_tracer(st, NULL, TRACE_FUN_ENABLED,
                             am_trace_status, am_undefined) == am_remove) {
         erts_set_system_seq_tracer(NULL, 0, erts_tracer_nil);
         st = erts_tracer_nil;
@@ -513,7 +512,7 @@ get_default_tracing(Uint *flagsp, ErtsTracer *tracerp,
 	*default_trace_flags &= ~TRACEE_FLAGS;
     } else {
         Eterm nif_res;
-        nif_res = call_enabled_tracer(NULL, *default_tracer,
+        nif_res = call_enabled_tracer(*default_tracer,
                                       NULL, TRACE_FUN_ENABLED,
                                       am_trace_status, am_undefined);
         switch (nif_res) {
@@ -915,8 +914,8 @@ seq_trace_update_send(Process *p)
     ASSERT((is_tuple(SEQ_TRACE_TOKEN(p)) || is_nil(SEQ_TRACE_TOKEN(p))));
     if (have_no_seqtrace(SEQ_TRACE_TOKEN(p)) ||
         (seq_tracer != NIL &&
-         call_enabled_tracer(NULL, seq_tracer, NULL,
-                             TRACE_FUN_ENABLED, am_trace_status,
+         call_enabled_tracer(seq_tracer, NULL,
+                             TRACE_FUN_ENABLED, am_seq_trace,
                              p ? p->common.id : am_undefined) != am_trace)
 #ifdef USE_VM_PROBES
 	 || (SEQ_TRACE_TOKEN(p) == am_have_dt_utag)
@@ -963,9 +962,9 @@ seq_trace_output_generic(Eterm token, Eterm msg, Uint type,
     ASSERT(is_tuple(token) || is_nil(token));
     if (token == NIL || (process && ERTS_TRACE_FLAGS(process) & F_SENSITIVE) ||
         ERTS_TRACER_IS_NIL(seq_tracer) ||
-        call_enabled_tracer(NULL, seq_tracer,
+        call_enabled_tracer(seq_tracer,
                             NULL, TRACE_FUN_ENABLED,
-                            am_trace_status,
+                            am_seq_trace,
                             process ? process->common.id : am_undefined) != am_trace) {
 	return;
     }
@@ -1186,7 +1185,11 @@ erts_call_trace(Process* p, BeamInstr mfa[3], Binary *match_spec,
 	 *     use process flags
 	 */
 	tracee_flags = &ERTS_TRACE_FLAGS(p);
+        /* Is is not ideal at all to call this check twice,
+           it should be optimized so that only one call is made. */
         if (!is_tracer_enabled(p, ERTS_PROC_LOCK_MAIN, &p->common, &tnif,
+                               TRACE_FUN_ENABLED, am_trace_status)
+            || !is_tracer_enabled(p, ERTS_PROC_LOCK_MAIN, &p->common, &tnif,
                     TRACE_FUN_E_CALL, am_call)) {
             return 0;
         }
@@ -1202,13 +1205,21 @@ erts_call_trace(Process* p, BeamInstr mfa[3], Binary *match_spec,
         }
 	meta_flags = F_TRACE_CALLS | F_NOW_TS;
 	tracee_flags = &meta_flags;
-        switch (call_enabled_tracer(p, *tracer,
-                                    &tnif, TRACE_FUN_T_CALL,
-                                    am_call, p->common.id)) {
+        switch (call_enabled_tracer(*tracer,
+                                    &tnif, TRACE_FUN_ENABLED,
+                                    am_trace_status, p->common.id)) {
         default:
         case am_remove: *tracer = erts_tracer_nil;
         case am_discard: return 0;
-        case am_trace: break;
+        case am_trace:
+            switch (call_enabled_tracer(*tracer,
+                                        &tnif, TRACE_FUN_T_CALL,
+                                        am_call, p->common.id)) {
+            default:
+            case am_discard: return 0;
+            case am_trace: break;
+            }
+            break;
         }
     }
 
@@ -1346,9 +1357,9 @@ trace_proc(Process *c_p, ErtsProcLocks c_p_locks,
            Process *t_p, Eterm what, Eterm data)
 {
     ErtsTracerNif *tnif = NULL;
-    if (is_tracer_enabled(c_p, c_p_locks, &t_p->common, &tnif,
+    if (is_tracer_enabled(NULL, 0, &t_p->common, &tnif,
                 TRACE_FUN_E_PROCS, what))
-        send_to_tracer_nif(c_p, &t_p->common, t_p->common.id, tnif, TRACE_FUN_T_PROCS,
+        send_to_tracer_nif(NULL, &t_p->common, t_p->common.id, tnif, TRACE_FUN_T_PROCS,
                            what, data, THE_NON_VALUE, am_true);
 }
 
@@ -1365,16 +1376,15 @@ trace_proc_spawn(Process *p, Eterm what, Eterm pid,
 		 Eterm mod, Eterm func, Eterm args)
 {
     ErtsTracerNif *tnif = NULL;
-    if (is_tracer_enabled(p, ERTS_PROC_LOCKS_ALL &
-                               ~(ERTS_PROC_LOCK_STATUS|ERTS_PROC_LOCK_TRACE),
-                               &p->common, &tnif, TRACE_FUN_E_PROCS, what)) {
+    if (is_tracer_enabled(NULL, 0,
+			  &p->common, &tnif, TRACE_FUN_E_PROCS, what)) {
         Eterm mfa;
         Eterm* hp;
 
         hp = HAlloc(p, 4);
         mfa = TUPLE3(hp, mod, func, args);
         hp += 4;
-        send_to_tracer_nif(p, &p->common, p->common.id, tnif, TRACE_FUN_T_PROCS,
+        send_to_tracer_nif(NULL, &p->common, p->common.id, tnif, TRACE_FUN_T_PROCS,
                            what, pid, mfa, am_true);
     }
 }
@@ -2896,23 +2906,28 @@ send_to_tracer_nif(Process *c_p, ErtsPTabElementCommon *t_p,
 }
 
 static ERTS_INLINE Eterm
-call_enabled_tracer(Process *c_p, const ErtsTracer tracer,
+call_enabled_tracer(const ErtsTracer tracer,
                     ErtsTracerNif **tnif_ret,
                     enum ErtsTracerOpt topt,
                     Eterm tag, Eterm t_p_id) {
     ErtsTracerNif *tnif = lookup_tracer_nif(tracer);
     if (tnif) {
-        Eterm argv[] = {tag, ERTS_TRACER_STATE(tracer), t_p_id};
+        Eterm argv[] = {tag, ERTS_TRACER_STATE(tracer), t_p_id},
+            ret;
         topt = (tnif->tracers[topt].cb) ? topt : TRACE_FUN_ENABLED;
         ASSERT(topt < NIF_TRACER_TYPES);
         ASSERT(tnif->tracers[topt].cb != NULL);
         if (tnif_ret) *tnif_ret = tnif;
-        return erts_nif_call_function(c_p, NULL, tnif->nif_mod,
-                                      tnif->tracers[topt].cb,
-                                      tnif->tracers[topt].arity,
-                                      argv);
+        ret = erts_nif_call_function(NULL, NULL, tnif->nif_mod,
+                                     tnif->tracers[topt].cb,
+                                     tnif->tracers[topt].arity,
+                                     argv);
+        if (tag == am_trace_status && ret != am_remove)
+            return am_trace;
+        ASSERT(tag == am_trace_status || ret != am_remove);
+        return ret;
     }
-    return am_remove;
+    return tag == am_trace_status ? am_remove : am_discard;
 }
 
 static int
@@ -2937,12 +2952,12 @@ is_tracer_enabled(Process* c_p, ErtsProcLocks c_p_locks,
     }
 #endif
 
-    nif_result = call_enabled_tracer(c_p, t_p->tracer, tnif_ret, topt, tag, t_p->id);
+    nif_result = call_enabled_tracer(t_p->tracer, tnif_ret, topt, tag, t_p->id);
     switch (nif_result) {
     case am_discard: return 0;
     case am_trace: return 1;
     case THE_NON_VALUE:
-    case am_remove: break;
+    case am_remove: ASSERT(tag == am_trace_status); break;
     default:
         /* only am_remove should be returned, but if
            something else is returned we fall-through
@@ -2973,19 +2988,14 @@ is_tracer_enabled(Process* c_p, ErtsProcLocks c_p_locks,
     return 0;
 }
 
-int erts_is_tracer_proc_enabled(Process* c_p, ErtsProcLocks c_p_locks,
-                                ErtsPTabElementCommon *t_p, Eterm type)
-{
-    return is_tracer_enabled(c_p, c_p_locks, t_p, NULL, TRACE_FUN_ENABLED, am_trace_status);
-}
-
-int erts_is_tracer_enabled(Process *c_p, const ErtsTracer tracer)
+int erts_is_tracer_enabled(const ErtsTracer tracer, ErtsPTabElementCommon *t_p)
 {
     ErtsTracerNif *tnif = lookup_tracer_nif(tracer);
     if (tnif) {
-        Eterm nif_result = call_enabled_tracer(c_p, tracer, &tnif,
-                                               TRACE_FUN_ENABLED, am_trace_status,
-                                               c_p->common.id);
+        Eterm nif_result = call_enabled_tracer(tracer, &tnif,
+                                               TRACE_FUN_ENABLED,
+                                               am_trace_status,
+                                               t_p->id);
         switch (nif_result) {
         case am_discard:
         case am_trace: return 1;
@@ -2996,6 +3006,20 @@ int erts_is_tracer_enabled(Process *c_p, const ErtsTracer tracer)
     return 0;
 }
 
+int erts_is_tracer_proc_enabled(Process* c_p, ErtsProcLocks c_p_locks,
+                                ErtsPTabElementCommon *t_p)
+{
+    return is_tracer_enabled(c_p, c_p_locks, t_p, NULL, TRACE_FUN_ENABLED,
+                             am_trace_status);
+}
+
+int erts_is_tracer_proc_enabled_send(Process* c_p, ErtsProcLocks c_p_locks,
+                                     ErtsPTabElementCommon *t_p)
+{
+    return is_tracer_enabled(c_p, c_p_locks, t_p, NULL, TRACE_FUN_T_SEND, am_send);
+}
+
+
 void erts_tracer_replace(ErtsPTabElementCommon *t_p, const ErtsTracer tracer)
 {
 #if defined(ERTS_SMP) && defined(ERTS_ENABLE_LOCK_CHECK)
diff --git a/erts/emulator/beam/erl_trace.h b/erts/emulator/beam/erl_trace.h
index de4beadb7e..0a0c5fc913 100644
--- a/erts/emulator/beam/erl_trace.h
+++ b/erts/emulator/beam/erl_trace.h
@@ -193,8 +193,10 @@ int erts_finish_breakpointing(void);
 
 /* Nif tracer functions */
 int erts_is_tracer_proc_enabled(Process *c_p, ErtsProcLocks c_p_locks,
-                                ErtsPTabElementCommon *t_p, Eterm type);
-int erts_is_tracer_enabled(Process *c_p, const ErtsTracer tracer);
+                                ErtsPTabElementCommon *t_p);
+int erts_is_tracer_proc_enabled_send(Process* c_p, ErtsProcLocks c_p_locks,
+                                     ErtsPTabElementCommon *t_p);
+int erts_is_tracer_enabled(const ErtsTracer tracer, ErtsPTabElementCommon *t_p);
 Eterm erts_tracer_to_term(Process *p, ErtsTracer tracer);
 ErtsTracer erts_term_to_tracer(Eterm prefix, Eterm term);
 void erts_tracer_replace(ErtsPTabElementCommon *t_p,
@@ -224,9 +226,4 @@ ERTS_DECLARE_DUMMY(erts_tracer_nil) = NIL;
 #define ERTS_TRACER_FROM_ETERM(termp) \
     ((ErtsTracer*)(termp))
 
-#define ERTS_TRACER_PROC_IS_ENABLED(PROC)                               \
-    (!ERTS_TRACER_IS_NIL(ERTS_TRACER(PROC))                             \
-     && erts_is_tracer_proc_enabled(PROC, ERTS_PROC_LOCK_MAIN,          \
-                                    &(PROC)->common, am_trace_status))
-
 #endif /* ERL_TRACE_H__ */
diff --git a/erts/emulator/beam/io.c b/erts/emulator/beam/io.c
index ef326fafec..0377f6cb5e 100644
--- a/erts/emulator/beam/io.c
+++ b/erts/emulator/beam/io.c
@@ -5941,7 +5941,7 @@ driver_deliver_term(Port *prt, Eterm to, ErlDrvTermData* data, int len)
     if (!rp) {
         if (!prt || !IS_TRACED_FL(prt, F_TRACE_SEND))
             goto done;
-        if (!erts_is_tracer_proc_enabled(NULL, 0, &prt->common, am_send))
+        if (!erts_is_tracer_proc_enabled_send(NULL, 0, &prt->common))
             goto done;
 
 	res = -2;