From: Claudio Jeker Subject: Re: remove flag setting from proc_wakeup() To: David Gwynne Cc: tech@openbsd.org, visa@openbsd.org Date: Wed, 30 Apr 2025 10:13:24 +0200 On Wed, Apr 30, 2025 at 04:40:26PM +1000, David Gwynne wrote: > On Wed, Apr 30, 2025 at 07:17:18AM +0200, Claudio Jeker wrote: > > On Wed, Apr 30, 2025 at 03:05:08PM +1000, David Gwynne wrote: > > > 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: > > > > > > > Maybe adding a comment why this is safe is enough. It is true that > > P_TIMEOUTRAN solves this synchronisation in a better way. > > I plan to push the SCHED_LOCK into wakeup_proc() so the diff below does > > not really help. > > ok. i mean, i understand. > > 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 06:38:42 -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 06:38:42 -0000 > @@ -533,8 +533,12 @@ sleep_signal_check(struct proc *p, int a > return 0; > } > > +/* > + * If process hasn't been awakened (wchan non-zero), undo the sleep. > + * If proc is stopped, just unsleep so it will remain stopped. > + */ > int > -wakeup_proc(struct proc *p, int flags) > +wakeup_proc(struct proc *p) > { > int awakened = 0; > > @@ -542,8 +546,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); > @@ -558,22 +560,30 @@ wakeup_proc(struct proc *p, int flags) > > > /* > - * Implement timeout for tsleep. > - * If process hasn't been awakened (wchan non-zero), > - * set timeout flag and undo the sleep. If proc > - * is stopped, just unsleep so it will remain stopped. > + * This is the timeout handler that wakes up procs that only want > + * to sleep for a period of time rather than forever (until they get > + * a wakeup from somewhere else). It is only scheduled and used by > + * sleep_finish(), which coordinates with this handler via the P_TIMEOUT > + * and P_TIMEOUTRAN flags. > */ > void > endtsleep(void *arg) > { > struct proc *p = arg; > + int awakened; > + int flags; > > SCHED_LOCK(); > - wakeup_proc(p, P_TIMEOUT); > + awakened = wakeup_proc(p); > SCHED_UNLOCK(); > > - atomic_setbits_int(&p->p_flag, P_TIMEOUTRAN); > - /* do not alter the proc after this point */ > + flags = P_TIMEOUTRAN; > + if (awakened) > + SET(flags, P_TIMEOUT); > + > + /* Let sleep_finish() proceed. */ > + atomic_setbits_int(&p->p_flag, flags); > + /* Do not alter the proc after this point. */ > } > > /* > @@ -865,7 +875,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 06:38:42 -0000 > @@ -95,11 +95,20 @@ struct pgrp { > * generation counter. Code should use tu_enter() and tu_leave() for this. > * The process ps_tu structure is locked by the ps_mtx. > */ > +#define TU_UTICKS 0 /* Statclock hits in user mode. */ > +#define TU_STICKS 1 /* Statclock hits in system mode. */ > +#define TU_ITICKS 2 /* Statclock hits processing intr. */ > +#define TU_TICKS_COUNT 3 > + > struct tusage { > uint64_t tu_gen; /* generation counter */ > - uint64_t tu_uticks; /* Statclock hits in user mode. */ > - uint64_t tu_sticks; /* Statclock hits in system mode. */ > - uint64_t tu_iticks; /* Statclock hits processing intr. */ > + uint64_t tu_ticks[TU_TICKS_COUNT]; > +#define tu_uticks tu_ticks[TU_UTICKS] > +#define tu_sticks tu_ticks[TU_STICKS] > +#define tu_iticks tu_ticks[TU_ITICKS] > + uint64_t tu_ixrss; > + uint64_t tu_idrss; > + uint64_t tu_isrss; > struct timespec tu_runtime; /* Realtime. */ > }; > This bit is unrelated. If you skip this on commit then OK claudio@ > @@ -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); > > -- :wq Claudio