[Xenomai] [PATCH 0/6] Cobalt fixes (CPU affinity handling, cyclictest)

[Xenomai] [PATCH 0/6] Cobalt fixes (CPU affinity handling, cyclictest)

[Philippe Gerum]

This series mainly fixes CPU affinity issues when the set of rt CPUs
is restricted via xenomai.supported_cpus=<mask>, specifically:

- handle user requests for domain migration to the head stage over
  non-rt CPUs gracefully. This builds on the I-pipe patch sent earlier:

  sched/core: ipipe: do not panic on failed migration to the head stage

- set the default CPU affinity for userland apps to the restricted CPU
  set if given on the kernel command line (instead of any
  CPU). Typically, running switchtest on a system with a restricted rt
  CPU set without passing --cpu-affinity should implicitly select
  available (rt) CPUs exclusively.

As I have been running benchmarks lately, I fixed a bug in cyclictest
which caused weird latency results with long-running tests too.

Philippe Gerum (6):
  demo/cyclictest: turn on FIFO mode by default, detecting spurious
    relaxes
  cobalt/thread: handle case of invalid domain migration over non-rt CPU
  cobalt/intr: IRQ affinity depends on xnsched_realtime_cpus
  cobalt/sched: fix mismatches between supported CPUs and affinity set
  boilerplate/setup: cobalt: retrieve the default CPU affinity
  demo/cyclictest: fix time delta calculation

 demo/posix/cyclictest/cyclictest.c | 82 ++++++++++++++++++++++++++++++++------
 include/cobalt/kernel/sched.h      | 10 +++++
 kernel/cobalt/intr.c               |  2 +-
 kernel/cobalt/posix/process.c      | 21 ++++++----
 kernel/cobalt/posix/sched.c        |  2 +-
 kernel/cobalt/thread.c             |  1 +
 lib/boilerplate/setup.c            | 33 ++++++++++++++-
 7 files changed, 126 insertions(+), 25 deletions(-)

-- 
2.14.4

[Xenomai] [PATCH 1/6] demo/cyclictest: turn on FIFO mode by default, detecting spurious relaxes

[Philippe Gerum]

Make sure people do run the cyclictest the way it is intended to with
Xenomai (i.e. using Xenomai's rt scheduler...).

Also, detect and bail out upon spurious relaxes in dual kernel mode,
so that latency results are dismissed as they should until the bug is
fixed.
---
 demo/posix/cyclictest/cyclictest.c | 67 ++++++++++++++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 7 deletions(-)

diff --git a/demo/posix/cyclictest/cyclictest.c b/demo/posix/cyclictest/cyclictest.c
index 31d9e5d0b..87f313836 100644
--- a/demo/posix/cyclictest/cyclictest.c
+++ b/demo/posix/cyclictest/cyclictest.c
@@ -343,16 +343,16 @@ static inline int tsgreater(struct timespec *a, struct timespec *b)
 static inline int64_t calcdiff(struct timespec t1, struct timespec t2)
 {
 	int64_t diff;
-	diff = USEC_PER_SEC * (long long)((int) t1.tv_sec - (int) t2.tv_sec);
-	diff += ((int) t1.tv_nsec - (int) t2.tv_nsec) / 1000;
+	diff = USEC_PER_SEC * (long long)(t1.tv_sec - t2.tv_sec);
+	diff += (t1.tv_nsec - t2.tv_nsec) / 1000;
 	return diff;
 }
 
 static inline int64_t calcdiff_ns(struct timespec t1, struct timespec t2)
 {
 	int64_t diff;
-	diff = NSEC_PER_SEC * (int64_t)((int) t1.tv_sec - (int) t2.tv_sec);
-	diff += ((int) t1.tv_nsec - (int) t2.tv_nsec);
+	diff = NSEC_PER_SEC * (int64_t)(t1.tv_sec - t2.tv_sec);
+	diff += (t1.tv_nsec - t2.tv_nsec);
 	return diff;
 }
 
@@ -856,6 +856,10 @@ void *timerthread(void *param)
 
 	stat->threadstarted++;
 
+#ifdef CONFIG_XENO_COBALT
+	if (pthread_setmode_np(0, PTHREAD_WARNSW, NULL))
+		fatal("pthread_setmode_np()");
+#endif
 	while (!shutdown) {
 
 		uint64_t diff;
@@ -1064,7 +1068,7 @@ static void display_help(int error)
 	       "-N       --nsecs           print results in ns instead of us (default us)\n"
 	       "-o RED   --oscope=RED      oscilloscope mode, reduce verbose output by RED\n"
 	       "-O TOPT  --traceopt=TOPT   trace option\n"
-	       "-p PRIO  --prio=PRIO       priority of highest prio thread\n"
+	       "-p PRIO  --prio=PRIO       priority of highest prio thread (defaults to 99)\n"
 	       "-P       --preemptoff      Preempt off tracing (used with -b)\n"
 	       "-q       --quiet           print only a summary on exit\n"
 	       "	 --priospread       spread priority levels starting at specified value\n"
@@ -1110,8 +1114,8 @@ void application_usage(void)
 static int use_nanosleep;
 static int timermode = TIMER_ABSTIME;
 static int use_system;
-static int priority;
-static int policy = SCHED_OTHER;	/* default policy if not specified */
+static int priority = 99;
+static int policy = SCHED_FIFO;	/* default policy if not specified */
 static int num_threads = 1;
 static int max_cycles;
 static int clocksel = 0;
@@ -1805,9 +1809,52 @@ void *fifothread(void *param)
 	return NULL;
 }
 
+#ifdef CONFIG_XENO_COBALT
+#include <cobalt/uapi/syscall.h>
+
+static const char *reason_str[] = {
+	[SIGDEBUG_UNDEFINED] = "received SIGDEBUG for unknown reason",
+	[SIGDEBUG_MIGRATE_SIGNAL] = "received signal",
+	[SIGDEBUG_MIGRATE_SYSCALL] = "invoked syscall",
+	[SIGDEBUG_MIGRATE_FAULT] = "triggered fault",
+	[SIGDEBUG_MIGRATE_PRIOINV] = "affected by priority inversion",
+	[SIGDEBUG_NOMLOCK] = "process memory not locked",
+	[SIGDEBUG_WATCHDOG] = "watchdog triggered (period too short?)",
+	[SIGDEBUG_LOCK_BREAK] = "scheduler lock break",
+};
+
+static void sigdebug(int sig, siginfo_t *si, void *context)
+{
+	const char fmt[] = "%s, aborting.\n"
+		"(enabling CONFIG_XENO_OPT_DEBUG_TRACE_RELAX may help)\n";
+	unsigned int reason = sigdebug_reason(si);
+	int n __attribute__ ((unused));
+	static char buffer[256];
+
+	if (reason > SIGDEBUG_WATCHDOG)
+		reason = SIGDEBUG_UNDEFINED;
+
+	switch(reason) {
+	case SIGDEBUG_UNDEFINED:
+	case SIGDEBUG_NOMLOCK:
+	case SIGDEBUG_WATCHDOG:
+		n = snprintf(buffer, sizeof(buffer), "latency: %s\n",
+			     reason_str[reason]);
+		write_check(STDERR_FILENO, buffer, n);
+		exit(EXIT_FAILURE);
+	}
+
+	n = snprintf(buffer, sizeof(buffer), fmt, reason_str[reason]);
+	write_check(STDERR_FILENO, buffer, n);
+	signal(sig, SIG_DFL);
+	kill(getpid(), sig);
+}
+
+#endif
 
 int main(int argc, char **argv)
 {
+	struct sigaction sa __attribute__((unused));
 	sigset_t sigset;
 	int signum = SIGALRM;
 	int mode;
@@ -1952,6 +1999,12 @@ int main(int argc, char **argv)
 	signal(SIGINT, sighand);
 	signal(SIGTERM, sighand);
 	signal(SIGUSR1, sighand);
+#ifdef CONFIG_XENO_COBALT
+	sigemptyset(&sa.sa_mask);
+	sa.sa_sigaction = sigdebug;
+	sa.sa_flags = SA_SIGINFO;
+	sigaction(SIGDEBUG, &sa, NULL);
+#endif
 
 	parameters = calloc(num_threads, sizeof(struct thread_param *));
 	if (!parameters)
-- 
2.14.4

[Xenomai] [PATCH 2/6] cobalt/thread: handle case of invalid domain migration over non-rt CPU

[Philippe Gerum]

Attempting to migrate to the head domain while running on a CPU which
is not in part of the real-time set is a bug, and must be detected by
the core.

Furthermore, for this detection to work, the I-pipe must not BUG()
unconditionally when failing to schedule out such thread in
__ipipe_migrate_head(), but rather let the real-time core handle the
situation (i.e. Xenomai in xnthread_harden()).

Until both changes are in place, running a thread issuing a real-time
call over a non-RT CPU would trigger a BUG() assertion, e.g.:

With kernel parameter "xenomai.supported_cpus=2", running:

$ switchtest --cpu-affinity=0

would lead to:

[   11.681486] kernel BUG at kernel/sched/core.c:5816!
[   11.686343] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
---
 kernel/cobalt/posix/process.c | 19 ++++++++++++-------
 kernel/cobalt/thread.c        |  1 +
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/kernel/cobalt/posix/process.c b/kernel/cobalt/posix/process.c
index 9a2bf83c2..a708833ef 100644
--- a/kernel/cobalt/posix/process.c
+++ b/kernel/cobalt/posix/process.c
@@ -856,7 +856,7 @@ static int handle_setaffinity_event(struct ipipe_cpu_migration_data *d)
 	 * take place on behalf of the target thread itself while
 	 * running in secondary mode. Therefore, that thread needs to
 	 * go through secondary mode first, then move back to primary
-	 * mode, so that check_affinity() does the fixup work.
+	 * mode, so that affinity_ok() does the fixup work.
 	 *
 	 * We force this by sending a SIGSHADOW signal to the migrated
 	 * thread, asking it to switch back to primary mode from the
@@ -873,7 +873,7 @@ static int handle_setaffinity_event(struct ipipe_cpu_migration_data *d)
 	return KEVENT_PROPAGATE;
 }
 
-static inline void check_affinity(struct task_struct *p) /* nklocked, IRQs off */
+static inline bool affinity_ok(struct task_struct *p) /* nklocked, IRQs off */
 {
 	struct xnthread *thread = xnthread_from_task(p);
 	struct xnsched *sched;
@@ -912,12 +912,12 @@ static inline void check_affinity(struct task_struct *p) /* nklocked, IRQs off *
 		 * in xnthread_harden().
 		 */
 		xnthread_set_info(thread, XNCANCELD);
-		return;
+		return false;
 	}
 
 	sched = xnsched_struct(cpu);
 	if (sched == thread->sched)
-		return;
+		return true;
 
 	/*
 	 * The current thread moved to a supported real-time CPU,
@@ -929,6 +929,8 @@ static inline void check_affinity(struct task_struct *p) /* nklocked, IRQs off *
 
 	xnthread_run_handler_stack(thread, move_thread, cpu);
 	xnthread_migrate_passive(thread, sched);
+
+	return true;
 }
 
 #else /* !CONFIG_SMP */
@@ -940,7 +942,10 @@ static int handle_setaffinity_event(struct ipipe_cpu_migration_data *d)
 	return KEVENT_PROPAGATE;
 }
 
-static inline void check_affinity(struct task_struct *p) { }
+static inline bool affinity_ok(struct task_struct *p)
+{
+	return true;
+}
 
 #endif /* CONFIG_SMP */
 
@@ -955,8 +960,8 @@ void ipipe_migration_hook(struct task_struct *p) /* hw IRQs off */
 	 */
 	xnlock_get(&nklock);
 	xnthread_run_handler_stack(thread, harden_thread);
-	check_affinity(p);
-	xnthread_resume(thread, XNRELAX);
+	if (affinity_ok(p))
+		xnthread_resume(thread, XNRELAX);
 	xnlock_put(&nklock);
 
 	xnsched_run();
diff --git a/kernel/cobalt/thread.c b/kernel/cobalt/thread.c
index 2ffe1f8f0..04c0b62d3 100644
--- a/kernel/cobalt/thread.c
+++ b/kernel/cobalt/thread.c
@@ -1945,6 +1945,7 @@ int xnthread_harden(void)
 
 	ret = __ipipe_migrate_head();
 	if (ret) {
+		xnthread_test_cancel();
 		xnthread_set_sync_window(thread, XNRELAX);
 		return ret;
 	}
-- 
2.14.4

[Xenomai] [PATCH 3/6] cobalt/intr: IRQ affinity depends on xnsched_realtime_cpus

[Philippe Gerum]

cobalt_cpu_affinity defines the CPU set which may run Cobalt threads,
which is a subset of xnsched_realtime_cpus. I.e. we may handle IRQs
over CPUs which do not run threads.
---
 kernel/cobalt/intr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cobalt/intr.c b/kernel/cobalt/intr.c
index 69faa23ba..184ffad13 100644
--- a/kernel/cobalt/intr.c
+++ b/kernel/cobalt/intr.c
@@ -822,7 +822,7 @@ int xnintr_attach(struct xnintr *intr, void *cookie)
 	clear_irqstats(intr);
 
 #ifdef CONFIG_SMP
-	ipipe_set_irq_affinity(intr->irq, cobalt_cpu_affinity);
+	ipipe_set_irq_affinity(intr->irq, xnsched_realtime_cpus);
 #endif /* CONFIG_SMP */
 
 	raw_spin_lock(&intr->lock);
-- 
2.14.4

[Xenomai] [PATCH 4/6] cobalt/sched: fix mismatches between supported CPUs and affinity set

[Philippe Gerum]

xnsched_realtime_cpus is a misnomer defining the set of CPUs which may
handle timer events.

cobalt_cpu_affinity is a subset of xnsched_realtime_cpus which defines
which CPU(s) may run Cobalt threads.
---
 include/cobalt/kernel/sched.h | 10 ++++++++++
 kernel/cobalt/posix/process.c |  2 +-
 kernel/cobalt/posix/sched.c   |  2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/cobalt/kernel/sched.h b/include/cobalt/kernel/sched.h
index eab80c340..d2b3d0c63 100644
--- a/include/cobalt/kernel/sched.h
+++ b/include/cobalt/kernel/sched.h
@@ -259,6 +259,11 @@ static inline int xnsched_supported_cpu(int cpu)
 	return cpumask_test_cpu(cpu, &xnsched_realtime_cpus);
 }
 
+static inline int xnsched_threading_cpu(int cpu)
+{
+	return cpumask_test_cpu(cpu, &cobalt_cpu_affinity);
+}
+
 #else /* !CONFIG_SMP */
 
 static inline void xnsched_set_resched(struct xnsched *sched)
@@ -273,6 +278,11 @@ static inline int xnsched_supported_cpu(int cpu)
 	return 1;
 }
 
+static inline int xnsched_threading_cpu(int cpu)
+{
+	return 1;
+}
+
 #endif /* !CONFIG_SMP */
 
 #define for_each_realtime_cpu(cpu)		\
diff --git a/kernel/cobalt/posix/process.c b/kernel/cobalt/posix/process.c
index a708833ef..d0f2f3725 100644
--- a/kernel/cobalt/posix/process.c
+++ b/kernel/cobalt/posix/process.c
@@ -897,7 +897,7 @@ static inline bool affinity_ok(struct task_struct *p) /* nklocked, IRQs off */
 	 * in secondary mode. If so, do the fixups to reflect the
 	 * change.
 	 */
-	if (!xnsched_supported_cpu(cpu)) {
+	if (!xnsched_threading_cpu(cpu)) {
 		/*
 		 * The thread is about to switch to primary mode on a
 		 * non-rt CPU, which is damn wrong and hopeless.
diff --git a/kernel/cobalt/posix/sched.c b/kernel/cobalt/posix/sched.c
index eca16f764..103f85534 100644
--- a/kernel/cobalt/posix/sched.c
+++ b/kernel/cobalt/posix/sched.c
@@ -622,7 +622,7 @@ int __cobalt_sched_setconfig_np(int cpu, int policy,
 
 	trace_cobalt_sched_setconfig(cpu, policy, len);
 
-	if (cpu < 0 || cpu >= NR_CPUS || !xnsched_supported_cpu(cpu))
+	if (cpu < 0 || cpu >= NR_CPUS || !xnsched_threading_cpu(cpu))
 		return -EINVAL;
 
 	if (len == 0)
-- 
2.14.4

[Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Philippe Gerum]

Over Cobalt, make sure the default CPU affinity mask reflects the set
of CPUs available for running real-time threads
(i.e. /proc/xenomai/affinity).
---
 lib/boilerplate/setup.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/lib/boilerplate/setup.c b/lib/boilerplate/setup.c
index 8b363efee..3214d7f04 100644
--- a/lib/boilerplate/setup.c
+++ b/lib/boilerplate/setup.c
@@ -173,6 +173,8 @@ static int collect_cpu_affinity(const char *cpu_list)
 		return ret;
 	}
 
+	CPU_ZERO(&__base_setup_data.cpu_affinity);
+
 	s = n = strdup(cpu_list);
 	while ((range = strtok_r(n, ",", &range_p)) != NULL) {
 		if (*range == '\0')
@@ -232,6 +234,33 @@ fail:
 	return -EINVAL;
 }
 
+static void retrieve_default_cpu_affinity(void)
+{
+	CPU_ZERO(&__base_setup_data.cpu_affinity);
+
+#ifdef CONFIG_XENO_COBALT
+	/*
+	 * If the Cobalt core has restricted the CPU set, update our
+	 * mask accordingly.
+	 */
+	unsigned long cpumask;
+	FILE *fp;
+	int cpu;
+
+	fp = fopen("/proc/xenomai/affinity", "r");
+	if (fp == NULL)
+		return;	/* uhh? */
+
+	if (fscanf(fp, "%lx", &cpumask) == 1) {
+		for (cpu = 0; cpumask; cpumask >>= 1, cpu++)
+			if (cpumask & 1)
+				CPU_SET(cpu, &__base_setup_data.cpu_affinity);
+	}
+
+	fclose(fp);
+#endif
+}
+
 static inline char **prep_args(int argc, char *const argv[])
 {
 	char **uargv;
@@ -528,8 +557,8 @@ static void __xenomai_init(int *argcp, char *const **argvp, const char *me)
 	/* No ifs, no buts: we must be called over the main thread. */
 	assert(getpid() == __node_id);
 
-	/* Define default CPU affinity, i.e. no particular affinity. */
-	CPU_ZERO(&__base_setup_data.cpu_affinity);
+	/* Retrieve the default CPU affinity. */
+	retrieve_default_cpu_affinity();
 
 	/*
 	 * Parse the base options first, to bootstrap the core with
-- 
2.14.4

[Xenomai] [PATCH 6/6] demo/cyclictest: fix time delta calculation

[Philippe Gerum]

Otherwise, weird results could be seen when the second wraps, leading
to t1.nsec < t2.nsec, e.g.:

T: 0 ( 1107) P:98 I:1000 C:1886710 Min:      0 Act:    4 Avg:2147483647 Max:      -1
---
 demo/posix/cyclictest/cyclictest.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/demo/posix/cyclictest/cyclictest.c b/demo/posix/cyclictest/cyclictest.c
index 87f313836..ebe5461db 100644
--- a/demo/posix/cyclictest/cyclictest.c
+++ b/demo/posix/cyclictest/cyclictest.c
@@ -340,20 +340,23 @@ static inline int tsgreater(struct timespec *a, struct timespec *b)
 		(a->tv_sec == b->tv_sec && a->tv_nsec > b->tv_nsec));
 }
 
-static inline int64_t calcdiff(struct timespec t1, struct timespec t2)
+static inline int64_t calcdiff_ns(struct timespec t1, struct timespec t2)
 {
-	int64_t diff;
-	diff = USEC_PER_SEC * (long long)(t1.tv_sec - t2.tv_sec);
-	diff += (t1.tv_nsec - t2.tv_nsec) / 1000;
-	return diff;
+	struct timespec r;
+	
+	r.tv_sec = t1.tv_sec - t2.tv_sec;
+	r.tv_nsec = t1.tv_nsec - t2.tv_nsec;
+	if (r.tv_nsec < 0) {
+		r.tv_sec--;
+		r.tv_nsec += NSEC_PER_SEC;
+	}
+
+	return r.tv_sec * NSEC_PER_SEC + r.tv_nsec;
 }
 
-static inline int64_t calcdiff_ns(struct timespec t1, struct timespec t2)
+static inline int64_t calcdiff(struct timespec t1, struct timespec t2)
 {
-	int64_t diff;
-	diff = NSEC_PER_SEC * (int64_t)(t1.tv_sec - t2.tv_sec);
-	diff += (t1.tv_nsec - t2.tv_nsec);
-	return diff;
+	return calcdiff_ns(t1, t2) / 1000;
 }
 
 void traceopt(char *option)
-- 
2.14.4

Re: [Xenomai] [PATCH 0/6] Cobalt fixes (CPU affinity handling, cyclictest)

[Jan Kiszka]

On 01.10.18 15:56, Philippe Gerum wrote:
> This series mainly fixes CPU affinity issues when the set of rt CPUs
> is restricted via xenomai.supported_cpus=<mask>, specifically:
> 
> - handle user requests for domain migration to the head stage over
>    non-rt CPUs gracefully. This builds on the I-pipe patch sent earlier:
> 
>    sched/core: ipipe: do not panic on failed migration to the head stage
> 
> - set the default CPU affinity for userland apps to the restricted CPU
>    set if given on the kernel command line (instead of any
>    CPU). Typically, running switchtest on a system with a restricted rt
>    CPU set without passing --cpu-affinity should implicitly select
>    available (rt) CPUs exclusively.
> 
> As I have been running benchmarks lately, I fixed a bug in cyclictest
> which caused weird latency results with long-running tests too.
> 
> Philippe Gerum (6):
>    demo/cyclictest: turn on FIFO mode by default, detecting spurious
>      relaxes
>    cobalt/thread: handle case of invalid domain migration over non-rt CPU
>    cobalt/intr: IRQ affinity depends on xnsched_realtime_cpus
>    cobalt/sched: fix mismatches between supported CPUs and affinity set
>    boilerplate/setup: cobalt: retrieve the default CPU affinity
>    demo/cyclictest: fix time delta calculation
> 
>   demo/posix/cyclictest/cyclictest.c | 82 ++++++++++++++++++++++++++++++++------
>   include/cobalt/kernel/sched.h      | 10 +++++
>   kernel/cobalt/intr.c               |  2 +-
>   kernel/cobalt/posix/process.c      | 21 ++++++----
>   kernel/cobalt/posix/sched.c        |  2 +-
>   kernel/cobalt/thread.c             |  1 +
>   lib/boilerplate/setup.c            | 33 ++++++++++++++-
>   7 files changed, 126 insertions(+), 25 deletions(-)
> 

Will have a look. Already noticed: missing signed-off. In the kernel community, 
this sometimes indicated "not yet for merge", but I suppose you just forgot 
them. If you confirm that and I have no other remarks, I can fix that up while 
merging.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH 0/6] Cobalt fixes (CPU affinity handling, cyclictest)

[Philippe Gerum]

On 10/01/2018 04:05 PM, Jan Kiszka wrote:
> On 01.10.18 15:56, Philippe Gerum wrote:
>> This series mainly fixes CPU affinity issues when the set of rt CPUs
>> is restricted via xenomai.supported_cpus=<mask>, specifically:
>>
>> - handle user requests for domain migration to the head stage over
>>    non-rt CPUs gracefully. This builds on the I-pipe patch sent earlier:
>>
>>    sched/core: ipipe: do not panic on failed migration to the head stage
>>
>> - set the default CPU affinity for userland apps to the restricted CPU
>>    set if given on the kernel command line (instead of any
>>    CPU). Typically, running switchtest on a system with a restricted rt
>>    CPU set without passing --cpu-affinity should implicitly select
>>    available (rt) CPUs exclusively.
>>
>> As I have been running benchmarks lately, I fixed a bug in cyclictest
>> which caused weird latency results with long-running tests too.
>>
>> Philippe Gerum (6):
>>    demo/cyclictest: turn on FIFO mode by default, detecting spurious
>>      relaxes
>>    cobalt/thread: handle case of invalid domain migration over non-rt CPU
>>    cobalt/intr: IRQ affinity depends on xnsched_realtime_cpus
>>    cobalt/sched: fix mismatches between supported CPUs and affinity set
>>    boilerplate/setup: cobalt: retrieve the default CPU affinity
>>    demo/cyclictest: fix time delta calculation
>>
>>   demo/posix/cyclictest/cyclictest.c | 82
>> ++++++++++++++++++++++++++++++++------
>>   include/cobalt/kernel/sched.h      | 10 +++++
>>   kernel/cobalt/intr.c               |  2 +-
>>   kernel/cobalt/posix/process.c      | 21 ++++++----
>>   kernel/cobalt/posix/sched.c        |  2 +-
>>   kernel/cobalt/thread.c             |  1 +
>>   lib/boilerplate/setup.c            | 33 ++++++++++++++-
>>   7 files changed, 126 insertions(+), 25 deletions(-)
>>
> 
> Will have a look. Already noticed: missing signed-off. In the kernel
> community, this sometimes indicated "not yet for merge", but I suppose
> you just forgot them. If you confirm that and I have no other remarks, I
> can fix that up while merging.
> 

I confirm that I'm submitting this upstream, please fix.

-- 
Philippe.

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Jan Kiszka]

On 01.10.18 15:56, Philippe Gerum wrote:
> Over Cobalt, make sure the default CPU affinity mask reflects the set
> of CPUs available for running real-time threads
> (i.e. /proc/xenomai/affinity).

"This avoids that thread will be created on CPU that is not support by Xenomai 
and first needs to be migrated when it is hardened."

Correct? I'm a fan of rather describing the "why" than the "what" in commit log. 
The latter is often already in the code.

Jan

> ---
>   lib/boilerplate/setup.c | 33 +++++++++++++++++++++++++++++++--
>   1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/boilerplate/setup.c b/lib/boilerplate/setup.c
> index 8b363efee..3214d7f04 100644
> --- a/lib/boilerplate/setup.c
> +++ b/lib/boilerplate/setup.c
> @@ -173,6 +173,8 @@ static int collect_cpu_affinity(const char *cpu_list)
>   		return ret;
>   	}
>   
> +	CPU_ZERO(&__base_setup_data.cpu_affinity);
> +
>   	s = n = strdup(cpu_list);
>   	while ((range = strtok_r(n, ",", &range_p)) != NULL) {
>   		if (*range == '\0')
> @@ -232,6 +234,33 @@ fail:
>   	return -EINVAL;
>   }
>   
> +static void retrieve_default_cpu_affinity(void)
> +{
> +	CPU_ZERO(&__base_setup_data.cpu_affinity);
> +
> +#ifdef CONFIG_XENO_COBALT
> +	/*
> +	 * If the Cobalt core has restricted the CPU set, update our
> +	 * mask accordingly.
> +	 */
> +	unsigned long cpumask;
> +	FILE *fp;
> +	int cpu;
> +
> +	fp = fopen("/proc/xenomai/affinity", "r");
> +	if (fp == NULL)
> +		return;	/* uhh? */
> +
> +	if (fscanf(fp, "%lx", &cpumask) == 1) {
> +		for (cpu = 0; cpumask; cpumask >>= 1, cpu++)
> +			if (cpumask & 1)
> +				CPU_SET(cpu, &__base_setup_data.cpu_affinity);
> +	}
> +
> +	fclose(fp);
> +#endif
> +}
> +
>   static inline char **prep_args(int argc, char *const argv[])
>   {
>   	char **uargv;
> @@ -528,8 +557,8 @@ static void __xenomai_init(int *argcp, char *const **argvp, const char *me)
>   	/* No ifs, no buts: we must be called over the main thread. */
>   	assert(getpid() == __node_id);
>   
> -	/* Define default CPU affinity, i.e. no particular affinity. */
> -	CPU_ZERO(&__base_setup_data.cpu_affinity);
> +	/* Retrieve the default CPU affinity. */
> +	retrieve_default_cpu_affinity();
>   
>   	/*
>   	 * Parse the base options first, to bootstrap the core with
> 

-- 
-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Jan Kiszka]

On 02.10.18 17:26, Jan Kiszka wrote:
> On 01.10.18 15:56, Philippe Gerum wrote:
>> Over Cobalt, make sure the default CPU affinity mask reflects the set
>> of CPUs available for running real-time threads
>> (i.e. /proc/xenomai/affinity).
> 
> "This avoids that thread will be created on CPU that is not support by Xenomai 
> and first needs to be migrated when it is hardened."
> 

After proof-reading:

"This avoids that a thread will be created on a CPU that is not supported by 
Xenomai and first needs to be migrated when it is hardened."

> Correct? I'm a fan of rather describing the "why" than the "what" in commit log. 
> The latter is often already in the code.
> 
> Jan
> 
>> ---
>>   lib/boilerplate/setup.c | 33 +++++++++++++++++++++++++++++++--
>>   1 file changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/boilerplate/setup.c b/lib/boilerplate/setup.c
>> index 8b363efee..3214d7f04 100644
>> --- a/lib/boilerplate/setup.c
>> +++ b/lib/boilerplate/setup.c
>> @@ -173,6 +173,8 @@ static int collect_cpu_affinity(const char *cpu_list)
>>           return ret;
>>       }
>> +    CPU_ZERO(&__base_setup_data.cpu_affinity);
>> +
>>       s = n = strdup(cpu_list);
>>       while ((range = strtok_r(n, ",", &range_p)) != NULL) {
>>           if (*range == '\0')
>> @@ -232,6 +234,33 @@ fail:
>>       return -EINVAL;
>>   }
>> +static void retrieve_default_cpu_affinity(void)
>> +{
>> +    CPU_ZERO(&__base_setup_data.cpu_affinity);
>> +
>> +#ifdef CONFIG_XENO_COBALT
>> +    /*
>> +     * If the Cobalt core has restricted the CPU set, update our
>> +     * mask accordingly.
>> +     */
>> +    unsigned long cpumask;
>> +    FILE *fp;
>> +    int cpu;
>> +
>> +    fp = fopen("/proc/xenomai/affinity", "r");
>> +    if (fp == NULL)
>> +        return;    /* uhh? */
>> +
>> +    if (fscanf(fp, "%lx", &cpumask) == 1) {
>> +        for (cpu = 0; cpumask; cpumask >>= 1, cpu++)
>> +            if (cpumask & 1)
>> +                CPU_SET(cpu, &__base_setup_data.cpu_affinity);
>> +    }
>> +
>> +    fclose(fp);
>> +#endif
>> +}
>> +
>>   static inline char **prep_args(int argc, char *const argv[])
>>   {
>>       char **uargv;
>> @@ -528,8 +557,8 @@ static void __xenomai_init(int *argcp, char *const 
>> **argvp, const char *me)
>>       /* No ifs, no buts: we must be called over the main thread. */
>>       assert(getpid() == __node_id);
>> -    /* Define default CPU affinity, i.e. no particular affinity. */
>> -    CPU_ZERO(&__base_setup_data.cpu_affinity);
>> +    /* Retrieve the default CPU affinity. */
>> +    retrieve_default_cpu_affinity();
>>       /*
>>        * Parse the base options first, to bootstrap the core with
>>
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Philippe Gerum]

On 10/02/2018 05:26 PM, Jan Kiszka wrote:
> On 01.10.18 15:56, Philippe Gerum wrote:
>> Over Cobalt, make sure the default CPU affinity mask reflects the set
>> of CPUs available for running real-time threads
>> (i.e. /proc/xenomai/affinity).
> 
> "This avoids that thread will be created on CPU that is not support by
> Xenomai and first needs to be migrated when it is hardened."
> 
> Correct? I'm a fan of rather describing the "why" than the "what" in
> commit log. The latter is often already in the code.
> 

You are describing a side-effect of not having the default CPU affinity
right, which people who are not familiar with the code are unlikely to
get easily. On the other hand, saying that we want applications to use
the underlying kernel's CPU set as their default CPU affinity setting
can be understood by anyone who understands the basics of SMP.

So I believe the original log is correct.

-- 
Philippe.

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Jan Kiszka]

On 02.10.18 17:43, Philippe Gerum wrote:
> On 10/02/2018 05:26 PM, Jan Kiszka wrote:
>> On 01.10.18 15:56, Philippe Gerum wrote:
>>> Over Cobalt, make sure the default CPU affinity mask reflects the set
>>> of CPUs available for running real-time threads
>>> (i.e. /proc/xenomai/affinity).
>>
>> "This avoids that thread will be created on CPU that is not support by
>> Xenomai and first needs to be migrated when it is hardened."
>>
>> Correct? I'm a fan of rather describing the "why" than the "what" in
>> commit log. The latter is often already in the code.
>>
> 
> You are describing a side-effect of not having the default CPU affinity
> right, which people who are not familiar with the code are unlikely to
> get easily. On the other hand, saying that we want applications to use
> the underlying kernel's CPU set as their default CPU affinity setting
> can be understood by anyone who understands the basics of SMP.
> 
> So I believe the original log is correct.

The original log is not explain why it changes things, it's only explaining what 
it does.

Just describe what was the effect of not having the mask set, and I will 
complete this commit.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Philippe Gerum]

On 10/02/2018 05:38 PM, Jan Kiszka wrote:
> On 02.10.18 17:26, Jan Kiszka wrote:
>> On 01.10.18 15:56, Philippe Gerum wrote:
>>> Over Cobalt, make sure the default CPU affinity mask reflects the set
>>> of CPUs available for running real-time threads
>>> (i.e. /proc/xenomai/affinity).
>>
>> "This avoids that thread will be created on CPU that is not support by
>> Xenomai and first needs to be migrated when it is hardened."
>>
> 
> After proof-reading:
> 
> "This avoids that a thread will be created on a CPU that is not
> supported by Xenomai and first needs to be migrated when it is hardened."

Not really. The issue is not with the creation phase, we have
xnthread_pin_initial() making sure that any emerging thread switches to
a CPU from the rt set already.

The issue is with manual CPU migration outside of the rt set applied to
a thread, which leads to a denial from the core (automatic
cancellation). You may want to factor in commit 2dc780e42 when reviewing
the changes.

So yes, it looks like reading the code is not that obvious. I'll rework
the log.

-- 
Philippe.

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Jan Kiszka]

On 02.10.18 18:30, Philippe Gerum wrote:
> On 10/02/2018 05:38 PM, Jan Kiszka wrote:
>> On 02.10.18 17:26, Jan Kiszka wrote:
>>> On 01.10.18 15:56, Philippe Gerum wrote:
>>>> Over Cobalt, make sure the default CPU affinity mask reflects the set
>>>> of CPUs available for running real-time threads
>>>> (i.e. /proc/xenomai/affinity).
>>>
>>> "This avoids that thread will be created on CPU that is not support by
>>> Xenomai and first needs to be migrated when it is hardened."
>>>
>>
>> After proof-reading:
>>
>> "This avoids that a thread will be created on a CPU that is not
>> supported by Xenomai and first needs to be migrated when it is hardened."
> 
> Not really. The issue is not with the creation phase, we have
> xnthread_pin_initial() making sure that any emerging thread switches to
> a CPU from the rt set already.
> 
> The issue is with manual CPU migration outside of the rt set applied to
> a thread, which leads to a denial from the core (automatic
> cancellation). You may want to factor in commit 2dc780e42 when reviewing
> the changes.
> 
> So yes, it looks like reading the code is not that obvious. I'll rework
> the log.

Did you have a chance to reword this commit log? I'd like to push it to next, 
along with its siblings.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] [PATCH 5/6] boilerplate/setup: cobalt: retrieve the default CPU affinity

[Philippe Gerum]

On 10/09/2018 12:40 PM, Jan Kiszka wrote:
> On 02.10.18 18:30, Philippe Gerum wrote:
>> On 10/02/2018 05:38 PM, Jan Kiszka wrote:
>>> On 02.10.18 17:26, Jan Kiszka wrote:
>>>> On 01.10.18 15:56, Philippe Gerum wrote:
>>>>> Over Cobalt, make sure the default CPU affinity mask reflects the set
>>>>> of CPUs available for running real-time threads
>>>>> (i.e. /proc/xenomai/affinity).
>>>>
>>>> "This avoids that thread will be created on CPU that is not support by
>>>> Xenomai and first needs to be migrated when it is hardened."
>>>>
>>>
>>> After proof-reading:
>>>
>>> "This avoids that a thread will be created on a CPU that is not
>>> supported by Xenomai and first needs to be migrated when it is
>>> hardened."
>>
>> Not really. The issue is not with the creation phase, we have
>> xnthread_pin_initial() making sure that any emerging thread switches to
>> a CPU from the rt set already.
>>
>> The issue is with manual CPU migration outside of the rt set applied to
>> a thread, which leads to a denial from the core (automatic
>> cancellation). You may want to factor in commit 2dc780e42 when reviewing
>> the changes.
>>
>> So yes, it looks like reading the code is not that obvious. I'll rework
>> the log.
> 
> Did you have a chance to reword this commit log? I'd like to push it to
> next, along with its siblings.
> 

Not yet, sorry. You can push this one independently later if you can't
wait, the real bug is solved elsewhere in the series. This one simply
prevents the default affinity settings to trigger the termination of the
app, which now works without panicking.

-- 
Philippe.