Merge pull request #11781 from fjmolinas/pr_schedstatistics_refactor

core/schedstatistics: fix call to uninitialized xtimer

Makefile.dep

@@ -610,6 +610,7 @@ endif
 
 ifneq (,$(filter schedstatistics,$(USEMODULE)))
   USEMODULE += xtimer
+  USEMODULE += sched_cb
 endif
 
 ifneq (,$(filter arduino,$(USEMODULE)))

core/include/sched.h

@@ -219,13 +219,21 @@ typedef struct {
  */
 extern schedstat_t sched_pidlist[KERNEL_PID_LAST + 1];
 
+/**
+ *  @brief  Registers the sched statistics callback and sets laststart for
+ *          caller thread
+ */
+void init_schedstatistics(void);
+#endif /* MODULE_SCHEDSTATISTICS */
+
+#ifdef MODULE_SCHED_CB
 /**
  *  @brief  Register a callback that will be called on every scheduler run
  *
  *  @param[in] callback The callback functions the will be called
  */
-void sched_register_cb(void (*callback)(uint32_t, uint32_t));
+void sched_register_cb(void (*callback)(kernel_pid_t, kernel_pid_t));
-#endif /* MODULE_SCHEDSTATISTICS */
+#endif /* MODULE_SCHED_CB */
 
 #ifdef __cplusplus
 }

core/kernel_init.c

@@ -29,10 +29,6 @@
 
 #include "periph/pm.h"
 
-#ifdef MODULE_SCHEDSTATISTICS
-#include "sched.h"
-#endif
-
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
@@ -49,11 +45,6 @@ static void *main_trampoline(void *arg)
     auto_init();
 #endif
 
-#ifdef MODULE_SCHEDSTATISTICS
-    schedstat_t *ss = &sched_pidlist[thread_getpid()];
-    ss->laststart = 0;
-#endif
-
     LOG_INFO("main(): This is RIOT! (Version: " RIOT_VERSION ")\n");
 
     main();

core/sched.c

@@ -74,8 +74,10 @@ FORCE_USED_SECTION
 const uint8_t _tcb_name_offset = offsetof(thread_t, name);
 #endif
 
+#ifdef MODULE_SCHED_CB
+static void (*sched_cb) (kernel_pid_t active_thread, kernel_pid_t next_thread) = NULL;
+#endif
 #ifdef MODULE_SCHEDSTATISTICS
-static void (*sched_cb) (uint32_t timestamp, uint32_t value) = NULL;
 schedstat_t sched_pidlist[KERNEL_PID_LAST + 1];
 #endif
 
@@ -100,10 +102,6 @@ int __attribute__((used)) sched_run(void)
         return 0;
     }
 
-#ifdef MODULE_SCHEDSTATISTICS
-    uint32_t now = xtimer_now().ticks32;
-#endif
-
     if (active_thread) {
         if (active_thread->status == STATUS_RUNNING) {
             active_thread->status = STATUS_PENDING;
@@ -114,21 +112,14 @@ int __attribute__((used)) sched_run(void)
             LOG_WARNING("scheduler(): stack overflow detected, pid=%" PRIkernel_pid "\n", active_thread->pid);
         }
 #endif
-
-#ifdef MODULE_SCHEDSTATISTICS
-        schedstat_t *active_stat = &sched_pidlist[active_thread->pid];
-        if (active_stat->laststart) {
-            active_stat->runtime_ticks += now - active_stat->laststart;
-        }
-#endif
     }
 
-#ifdef MODULE_SCHEDSTATISTICS
+#ifdef MODULE_SCHED_CB
-    schedstat_t *next_stat = &sched_pidlist[next_thread->pid];
-    next_stat->laststart = now;
-    next_stat->schedules++;
     if (sched_cb) {
-        sched_cb(now, next_thread->pid);
+        /* Use `sched_active_pid` instead of `active_thread` since after `sched_task_exit()` is
+           called `active_thread` is set to NULL while `sched_active_thread` isn't updated until
+           `next_thread` is scheduled*/
+        sched_cb(sched_active_pid, next_thread->pid);
     }
 #endif
 
@@ -151,13 +142,6 @@ int __attribute__((used)) sched_run(void)
     return 1;
 }
 
-#ifdef MODULE_SCHEDSTATISTICS
-void sched_register_cb(void (*callback)(uint32_t, uint32_t))
-{
-    sched_cb = callback;
-}
-#endif
-
 void sched_set_status(thread_t *process, thread_status_t status)
 {
     if (status >= STATUS_ON_RUNQUEUE) {
@@ -221,3 +205,37 @@ NORETURN void sched_task_exit(void)
     sched_active_thread = NULL;
     cpu_switch_context_exit();
 }
+
+#ifdef MODULE_SCHED_CB
+void sched_register_cb(void (*callback)(kernel_pid_t, kernel_pid_t))
+{
+    sched_cb = callback;
+}
+#endif
+
+#ifdef MODULE_SCHEDSTATISTICS
+void sched_statistics_cb(kernel_pid_t active_thread, kernel_pid_t next_thread)
+{
+    uint32_t now = xtimer_now().ticks32;
+
+    /* Update active thread runtime, there is always an active thread since
+       first sched_run happens when main_trampoline gets scheduled */
+    schedstat_t *active_stat = &sched_pidlist[active_thread];
+    active_stat->runtime_ticks += now - active_stat->laststart;
+
+    /* Update next_thread stats */
+    schedstat_t *next_stat = &sched_pidlist[next_thread];
+    next_stat->laststart = now;
+    next_stat->schedules++;
+}
+
+void init_schedstatistics(void)
+{
+    /* Init laststart for the thread starting schedstatistics since the callback
+       wasn't registered when it was first scheduled */
+    schedstat_t *active_stat = &sched_pidlist[sched_active_pid];
+    active_stat->laststart = xtimer_now().ticks32;
+    active_stat->schedules = 1;
+    sched_register_cb(sched_statistics_cb);
+}
+#endif

makefiles/pseudomodules.inc.mk

@@ -72,6 +72,7 @@ PSEUDOMODULES += saul_gpio
 PSEUDOMODULES += saul_nrf_temperature
 PSEUDOMODULES += scanf_float
 PSEUDOMODULES += schedstatistics
+PSEUDOMODULES += sched_cb
 PSEUDOMODULES += semtech_loramac_rx
 PSEUDOMODULES += sock
 PSEUDOMODULES += sock_ip

sys/auto_init/auto_init.c

@@ -92,6 +92,10 @@
 #include "net/sock/dtls.h"
 #endif
 
+#ifdef MODULE_SCHEDSTATISTICS
+#include "sched.h"
+#endif
+
 #define ENABLE_DEBUG (0)
 #include "debug.h"
 
@@ -105,6 +109,9 @@ void auto_init(void)
     DEBUG("Auto init xtimer module.\n");
     xtimer_init();
 #endif
+#ifdef MODULE_SCHEDSTATISTICS
+    init_schedstatistics();
+#endif
 #ifdef MODULE_MCI
     DEBUG("Auto init mci module.\n");
     mci_initialize();