[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: Scheduler hack for multi-threaded processes
From:       Mark Kettenis <mark.kettenis () xs4all ! nl>
Date:       2016-03-23 20:35:50
Message-ID: 201603232035.u2NKZoof005307 () glazunov ! sibelius ! xs4all ! nl
[Download RAW message or body]

> Date: Mon, 21 Mar 2016 16:51:16 +0100 (CET)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> 
> > Date: Sat, 19 Mar 2016 13:53:07 +0100
> > From: Martin Pieuchot <mpi@openbsd.org>
> > 
> > Applications using multiple threads often call sched_yield(2) to
> > indicate that one of the threads cannot make any progress because
> > it is waiting for a resource held by another one.
> > 
> > One example of this scenario is the _spinlock() implementation of
> > our librthread.  But if you look on https://codesearch.debian.net
> > you can find much more use cases, notably MySQL, PostgreSQL, JDK,
> > libreoffice, etc.
> > 
> > Now the problem with our current scheduler is that the priority of
> > a thread decreases when it is the "curproc" of a CPU.  So the threads
> > that don't run and sched_yield(2) end up having a higher priority than
> > the thread holding the resource.  Which means that it's really hard for
> > such multi-threaded applications to make progress, resulting in a lot of
> > IPIs numbers.
> > That'd also explain why if you have a more CPUs, let's say 4 instead
> > of 2, your application will more likely make some progress and you'll
> > see less sluttering/freezing.
> > 
> > So what the diff below does is that it penalizes the threads from
> > multi-threaded applications such that progress can be made.  It is
> > inspired from the recent scheduler work done by Michal Mazurek on
> > tech@.
> > 
> > I experimented with various values for "p_priority" and this one is
> > the one that generates fewer # IPIs when watching a HD video on firefox. 
> > Because yes, with this diff, now I can.
> > 
> > I'd like to know if dereferencing ``p_p'' is safe without holding the
> > KERNEL_LOCK.
> > 
> > I'm also interested in hearing from more people using multi-threaded
> > applications.
> 
> This doesn't only change the sched_yield() behaviour, but also
> modifies how in-kernel yield() calls behave for threaded processes.
> That is probably not right.

So here is a diff that keeps yield() the same and adds the code in the
sched_yield(2) implementation instead.  It also changes the logic that
picks the priority to walk the complete threads listand pick the
highest priotity of all the threads.  That should be enough to make
sure the calling thread is scheduled behind all other threads from the
same process.  That does require us to grab the kernel lock though.
So this removes NOLOCK from sched_yield(2).  I don't think that is a
big issue.

Still improves video playback in firefox on my x1.


Index: syscalls.master
===================================================================
RCS file: /cvs/src/sys/kern/syscalls.master,v
retrieving revision 1.167
diff -u -p -r1.167 syscalls.master
--- syscalls.master	21 Mar 2016 22:41:29 -0000	1.167
+++ syscalls.master	23 Mar 2016 20:33:45 -0000
@@ -514,7 +514,7 @@
 #else
 297	UNIMPL
 #endif
-298	STD NOLOCK	{ int sys_sched_yield(void); }
+298	STD		{ int sys_sched_yield(void); }
 299	STD NOLOCK	{ pid_t sys_getthrid(void); }
 300	OBSOL		t32___thrsleep
 301	STD		{ int sys___thrwakeup(const volatile void *ident, \
Index: kern_synch.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.129
diff -u -p -r1.129 kern_synch.c
--- kern_synch.c	9 Mar 2016 13:38:50 -0000	1.129
+++ kern_synch.c	23 Mar 2016 20:33:45 -0000
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_synch.c,v 1.129 2016/03/09 13:38:50 mpi Exp $	*/
+/*	$openbsd: kern_synch.c,v 1.129 2016/03/09 13:38:50 mpi Exp $	*/
 /*	$NetBSD: kern_synch.c,v 1.37 1996/04/22 01:38:37 christos Exp $	*/
 
 /*
@@ -432,7 +432,24 @@ wakeup(const volatile void *chan)
 int
 sys_sched_yield(struct proc *p, void *v, register_t *retval)
 {
-	yield();
+	struct proc *q;
+	int s;
+
+	SCHED_LOCK(s);
+	/*
+	 * If one of the threads of a multi-threaded process called
+	 * sched_yield(2), drop its priority to ensure its siblings
+	 * can make some progress.
+	 */
+	p->p_priority = p->p_usrpri;
+	TAILQ_FOREACH(q, &p->p_p->ps_threads, p_thr_link)
+		p->p_priority = max(p->p_priority, q->p_priority);
+	p->p_stat = SRUN;
+	setrunqueue(p);
+	p->p_ru.ru_nvcsw++;
+	mi_switch();
+	SCHED_UNLOCK(s);
+
 	return (0);
 }
 

[prev in list] [next in list] [prev in thread] [next in thread] 

Configure | About | News | Add a list | Sponsored by KoreLogic