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