Skip to contentSkip to conversation

Waitqueue-free gatekeeper wakeup

RFC In review Jan Kiszka 2 participants 2 messages Jul 18, 2011 – Jul 20, 2011

lore
Share & tools
lore

Jan Kiszka

author
#0 lore raw

RFC 1/1 Waitqueue-free gatekeeper wakeup

Hi Philippe,

trying to decouple the PREEMPT-RT gatekeeper wakeup path from XNATOMIC (to fix the remaining races there), I wondered why we need a waitqueue here at all.

What about an approach like below, i.e. waking up the gatekeeper directly via wake_up_process? That could even be called from interrupt context. We should be able to avoid missing a wakeup by setting the task state to INTERRUPTIBLE before signaling the semaphore.

Am I missing something?

Jan

include/nucleus/sched.h +0 -1
index e251329..df8853b 100644
@@ -111,7 +111,6 @@ typedef struct xnsched {
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	struct task_struct *gatekeeper;
-	wait_queue_head_t gkwaitq;
 	struct semaphore gksync;
 	struct xnthread *gktarget;
 #endif
ksrc/nucleus/shadow.c +5 -32
index f6b1e16..238317a 100644
@@ -92,7 +92,6 @@ static struct __lostagerq {
 #define LO_SIGGRP_REQ 2
 #define LO_SIGTHR_REQ 3
 #define LO_UNMAP_REQ  4
-#define LO_GKWAKE_REQ 5
 		int type;
 		struct task_struct *task;
 		int arg;
@@ -759,9 +758,6 @@ static void lostage_handler(void *cookie)
 	int cpu, reqnum, type, arg, sig, sigarg;
 	struct __lostagerq *rq;
 	struct task_struct *p;
-#ifdef CONFIG_PREEMPT_RT
-	struct xnsched *sched;
-#endif
 
 	cpu = smp_processor_id();
 	rq = &lostagerq[cpu];
@@ -819,13 +815,6 @@ static void lostage_handler(void *cookie)
 		case LO_SIGGRP_REQ:
 			kill_proc(p->pid, arg, 1);
 			break;
-
-#ifdef CONFIG_PREEMPT_RT
-		case LO_GKWAKE_REQ:
-			sched = xnpod_sched_slot(cpu);
-			wake_up_interruptible_sync(&sched->gkwaitq);
-			break;
-#endif
 		}
 	}
 }
@@ -873,7 +862,6 @@ static inline int normalize_priority(int prio)
 static int gatekeeper_thread(void *data)
 {
 	struct task_struct *this_task = current;
-	DECLARE_WAITQUEUE(wait, this_task);
 	int cpu = (long)data;
 	struct xnsched *sched = xnpod_sched_slot(cpu);
 	struct xnthread *target;
@@ -886,12 +874,10 @@ static int gatekeeper_thread(void *data)
 	set_cpus_allowed(this_task, cpumask);
 	set_linux_task_priority(this_task, MAX_RT_PRIO - 1);
 
-	init_waitqueue_head(&sched->gkwaitq);
-	add_wait_queue_exclusive(&sched->gkwaitq, &wait);
+	set_current_state(TASK_INTERRUPTIBLE);
 	up(&sched->gksync);	/* Sync with xnshadow_mount(). */
 
 	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
 		up(&sched->gksync); /* Make the request token available. */
 		schedule();
 
@@ -937,6 +923,7 @@ static int gatekeeper_thread(void *data)
 			xnlock_put_irqrestore(&nklock, s);
 			xnpod_schedule();
 		}
+		set_current_state(TASK_INTERRUPTIBLE);
 	}
 
 	return 0;
@@ -1014,23 +1001,9 @@ redo:
 	thread->gksched = sched;
 	xnthread_set_info(thread, XNATOMIC);
 	set_current_state(TASK_INTERRUPTIBLE | TASK_ATOMICSWITCH);
-#ifndef CONFIG_PREEMPT_RT
-	/*
-	 * We may not hold the preemption lock across calls to
-	 * wake_up_*() services over fully preemptible kernels, since
-	 * tasks might sleep when contending for spinlocks. The wake
-	 * up call for the gatekeeper will happen later, over an APC
-	 * we kick in do_schedule_event() on the way out for the
-	 * hardening task.
-	 *
-	 * We could delay the wake up call over non-RT 2.6 kernels as
-	 * well, but not when running over 2.4 (scheduler innards
-	 * would not allow this, causing weirdnesses when hardening
-	 * tasks). So we always do the early wake up when running
-	 * non-RT, which includes 2.4.
-	 */
-	wake_up_interruptible_sync(&sched->gkwaitq);
-#endif
+
+	wake_up_process(sched->gatekeeper);
+
 	schedule();
 
 	/*
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
#1 lore raw
Quoted 6 lines · Jan Kiszka

Hi Philippe,

trying to decouple the PREEMPT-RT gatekeeper wakeup path from XNATOMIC (to fix the remaining races there), I wondered why we need a waitqueue here at all.

Quoted 6 lines

What about an approach like below, i.e. waking up the gatekeeper directly via wake_up_process? That could even be called from interrupt context. We should be able to avoid missing a wakeup by setting the task state to INTERRUPTIBLE before signaling the semaphore.

Am I missing something?

No, I think this should work. IIRC, the wait queue dates back when we did not have a strong synchro between the hardening code and the gk via the request token, i.e. the initial implementation over 2.4 kernels. So it is about time to question this.

Quoted 110 lines

Jan

include/nucleus/sched.h +0 -1
index e251329..df8853b 100644
@@ -111,7 +111,6 @@ typedef struct xnsched {
 
 #ifdef CONFIG_XENO_OPT_PERVASIVE
 	struct task_struct *gatekeeper;
-	wait_queue_head_t gkwaitq;
 	struct semaphore gksync;
 	struct xnthread *gktarget;
 #endif
ksrc/nucleus/shadow.c +5 -32
index f6b1e16..238317a 100644
@@ -92,7 +92,6 @@ static struct __lostagerq {
 #define LO_SIGGRP_REQ 2
 #define LO_SIGTHR_REQ 3
 #define LO_UNMAP_REQ  4
-#define LO_GKWAKE_REQ 5
 		int type;
 		struct task_struct *task;
 		int arg;
@@ -759,9 +758,6 @@ static void lostage_handler(void *cookie)
 	int cpu, reqnum, type, arg, sig, sigarg;
 	struct __lostagerq *rq;
 	struct task_struct *p;
-#ifdef CONFIG_PREEMPT_RT
-	struct xnsched *sched;
-#endif
 
 	cpu = smp_processor_id();
 	rq = &lostagerq[cpu];
@@ -819,13 +815,6 @@ static void lostage_handler(void *cookie)
 		case LO_SIGGRP_REQ:
 			kill_proc(p->pid, arg, 1);
 			break;
-
-#ifdef CONFIG_PREEMPT_RT
-		case LO_GKWAKE_REQ:
-			sched = xnpod_sched_slot(cpu);
-			wake_up_interruptible_sync(&sched->gkwaitq);
-			break;
-#endif
 		}
 	}
 }
@@ -873,7 +862,6 @@ static inline int normalize_priority(int prio)
 static int gatekeeper_thread(void *data)
 {
 	struct task_struct *this_task = current;
-	DECLARE_WAITQUEUE(wait, this_task);
 	int cpu = (long)data;
 	struct xnsched *sched = xnpod_sched_slot(cpu);
 	struct xnthread *target;
@@ -886,12 +874,10 @@ static int gatekeeper_thread(void *data)
 	set_cpus_allowed(this_task, cpumask);
 	set_linux_task_priority(this_task, MAX_RT_PRIO - 1);
 
-	init_waitqueue_head(&sched->gkwaitq);
-	add_wait_queue_exclusive(&sched->gkwaitq, &wait);
+	set_current_state(TASK_INTERRUPTIBLE);
 	up(&sched->gksync);	/* Sync with xnshadow_mount(). */
 
 	for (;;) {
-		set_current_state(TASK_INTERRUPTIBLE);
 		up(&sched->gksync); /* Make the request token available. */
 		schedule();
 
@@ -937,6 +923,7 @@ static int gatekeeper_thread(void *data)
 			xnlock_put_irqrestore(&nklock, s);
 			xnpod_schedule();
 		}
+		set_current_state(TASK_INTERRUPTIBLE);
 	}
 
 	return 0;
@@ -1014,23 +1001,9 @@ redo:
 	thread->gksched = sched;
 	xnthread_set_info(thread, XNATOMIC);
 	set_current_state(TASK_INTERRUPTIBLE | TASK_ATOMICSWITCH);
-#ifndef CONFIG_PREEMPT_RT
-	/*
-	 * We may not hold the preemption lock across calls to
-	 * wake_up_*() services over fully preemptible kernels, since
-	 * tasks might sleep when contending for spinlocks. The wake
-	 * up call for the gatekeeper will happen later, over an APC
-	 * we kick in do_schedule_event() on the way out for the
-	 * hardening task.
-	 *
-	 * We could delay the wake up call over non-RT 2.6 kernels as
-	 * well, but not when running over 2.4 (scheduler innards
-	 * would not allow this, causing weirdnesses when hardening
-	 * tasks). So we always do the early wake up when running
-	 * non-RT, which includes 2.4.
-	 */
-	wake_up_interruptible_sync(&sched->gkwaitq);
-#endif
+
+	wake_up_process(sched->gatekeeper);
+
 	schedule();
 
 	/*
Signature · 1 line
Philippe.