Date: Wed, 24 Jan 2018 14:42:47 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Wojciech Macek <wma@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328320 - head/sys/kern Message-ID: <de3b1b93-0e2c-d218-7768-f42a364aa0c4@selasky.org> In-Reply-To: <201801240754.w0O7s57E063486@repo.freebsd.org> References: <201801240754.w0O7s57E063486@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 01/24/18 08:54, Wojciech Macek wrote: > Author: wma > Date: Wed Jan 24 07:54:05 2018 > New Revision: 328320 > URL: https://svnweb.freebsd.org/changeset/base/328320 > > Log: > ULE: provide defaults to ts_cpu > > Fix a bug when the system has no CPU 0. When created, threads were implicitly assigned to CPU 0. > This had no practical effect since a real CPU was chosen immediately by the scheduler. However, > on systems without a CPU 0, sched_ule attempted to access the scheduler queue of the "old" CPU > when assigned the initial choice of the old one. This caused an attempt to use illegal memory > and a crash (or, more usually, a deadlock). Fix this by assigned new threads to the BSP > explicitly and add some asserts to see that this problem does not recur. > > Authored by: Nathan Whitehorn <nwhitehorn@freebsd.org> > Submitted by: Wojciech Macek <wma@semihalf.com> > Obtained from: Semihalf > Differential revision: https://reviews.freebsd.org/D13932 > > Modified: > head/sys/kern/sched_ule.c > > Modified: head/sys/kern/sched_ule.c > ============================================================================== > --- head/sys/kern/sched_ule.c Wed Jan 24 07:01:44 2018 (r328319) > +++ head/sys/kern/sched_ule.c Wed Jan 24 07:54:05 2018 (r328320) > @@ -1405,6 +1405,7 @@ sched_setup(void *dummy) > > /* Add thread0's load since it's running. */ > TDQ_LOCK(tdq); > + td_get_sched(&thread0)->ts_cpu = curcpu; /* Something valid to start */ > thread0.td_lock = TDQ_LOCKPTR(TDQ_SELF()); > tdq_load_add(tdq, &thread0); > tdq->tdq_lowpri = thread0.td_priority; > @@ -2454,6 +2455,7 @@ sched_add(struct thread *td, int flags) > * Pick the destination cpu and if it isn't ours transfer to the > * target cpu. > */ > + td_get_sched(td)->ts_cpu = curcpu; /* Pick something valid to start */ > cpu = sched_pickcpu(td, flags); > tdq = sched_setcpu(td, cpu, flags); > tdq_add(tdq, td, flags); > > Hi Wojciech, I believe this chunk is wrong and the same like in r326218. See the explanation for r326376 as pasted below. Can you please revert? Further refer to existing discussions about "r326218" on the SVN commit list. Maybe you missed the point of r326376 when integrating? --HPS > Revision 326376 - (view) (download) (annotate) - [select for diffs] > Modified Wed Nov 29 23:28:40 2017 UTC (7 weeks, 6 days ago) by hselasky > File length: 80183 byte(s) > Diff to previous 326271 > > The sched_add() function is not only used when the thread is initially > started, but also by the turnstiles to mark a thread as runnable for > all locks, for instance sleepqueues do: > setrunnable()->sched_wakeup()->sched_add() > > In r326218 code was added to allow booting from non-zero CPU numbers > by setting the ts_cpu field inside the ULE scheduler's sched_add() > function. This had an undesired side-effect that prior sched_pin() and > sched_bind() calls got disregarded. This patch fixes the > initialization of the ts_cpu field for the ULE scheduler to only > happen once when the initial thread is constructed during system > init. Forking will then later on ensure that a valid ts_cpu value gets > copied to all children. > > Reviewed by: jhb, kib > Discussed with: nwhitehorn > MFC after: 1 month > Differential revision: https://reviews.freebsd.org/D13298 > Sponsored by: Mellanox Technologies >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?de3b1b93-0e2c-d218-7768-f42a364aa0c4>