On Wed, Apr 30, 2025 at 06:55:31AM +0200, Claudio Jeker wrote:
> On Wed, Apr 30, 2025 at 10:51:18AM +1000, David Gwynne wrote:
> > only endtsleep() sets flags via proc_wakeup, but it sets it's own flags
> > later. we can use the return value from proc_wakeup and the existing
> > flag setting to get the same effect.
> > 
> > ok?
> 
> I'm unsure about this. I did this change in perparation for fine grained
> SCHED_LOCK and then we may have the pronlem that the proc starts running
> before P_TIMEOUT is set. This could already be a problem now since you set
> the flag after SCHED_UNLOCK.

the proc may be running again, but we know it will be stuck in
sleep_finish() waiting for the P_TIMEOUTRAN flag to be set.

if it worries you a lot we can keep the flags change inside the
SCHED_LOCK like this:

Index: dev/pci/drm/drm_linux.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
diff -u -p -r1.122 drm_linux.c
--- dev/pci/drm/drm_linux.c	10 Mar 2025 09:28:56 -0000	1.122
+++ dev/pci/drm/drm_linux.c	30 Apr 2025 04:59:08 -0000
@@ -164,7 +164,7 @@ wake_up_process(struct proc *p)
 	int rv;
 
 	SCHED_LOCK();
-	rv = wakeup_proc(p, 0);
+	rv = wakeup_proc(p);
 	SCHED_UNLOCK();
 	return rv;
 }
Index: kern/kern_synch.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_synch.c,v
diff -u -p -r1.221 kern_synch.c
--- kern/kern_synch.c	30 Apr 2025 00:26:02 -0000	1.221
+++ kern/kern_synch.c	30 Apr 2025 04:59:08 -0000
@@ -534,7 +534,7 @@ sleep_signal_check(struct proc *p, int a
 }
 
 int
-wakeup_proc(struct proc *p, int flags)
+wakeup_proc(struct proc *p)
 {
 	int awakened = 0;
 
@@ -542,8 +542,6 @@ wakeup_proc(struct proc *p, int flags)
 
 	if (p->p_wchan != NULL) {
 		awakened = 1;
-		if (flags)
-			atomic_setbits_int(&p->p_flag, flags);
 #ifdef DIAGNOSTIC
 		if (p->p_stat != SSLEEP && p->p_stat != SSTOP)
 			panic("thread %d p_stat is %d", p->p_tid, p->p_stat);
@@ -567,13 +565,13 @@ void
 endtsleep(void *arg)
 {
 	struct proc *p = arg;
+	int flags = P_TIMEOUTRAN;
 
 	SCHED_LOCK();
-	wakeup_proc(p, P_TIMEOUT);
+	if (wakeup_proc(p))
+		SET(flags, P_TIMEOUT);
+	atomic_setbits_int(&p->p_flag, flags);
 	SCHED_UNLOCK();
-
-	atomic_setbits_int(&p->p_flag, P_TIMEOUTRAN);
-	/* do not alter the proc after this point */
 }
 
 /*
@@ -865,7 +863,7 @@ tslp_wakeups(struct tslpqueue *tslpq)
 	TAILQ_FOREACH_SAFE(entry, tslpq, tslp_link, nentry) {
 		p = entry->tslp_p;
 		entry->tslp_p = NULL;
-		wakeup_proc(p, 0);
+		wakeup_proc(p);
 	}
 	SCHED_UNLOCK();
 }
Index: sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
diff -u -p -r1.385 proc.h
--- sys/proc.h	30 Apr 2025 00:45:08 -0000	1.385
+++ sys/proc.h	30 Apr 2025 04:59:08 -0000
@@ -567,7 +576,7 @@ void	procinit(void);
 void	setpriority(struct proc *, uint32_t, uint8_t);
 void	setrunnable(struct proc *);
 void	endtsleep(void *);
-int	wakeup_proc(struct proc *, int);
+int	wakeup_proc(struct proc *);
 void	unsleep(struct proc *);
 void	reaper(void *);
 __dead void exit1(struct proc *, int, int, int);