Date: Tue, 26 Jan 2010 12:39:02 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Warner Losh <imp@freebsd.org>, Marcel Moolenaar <marcel@freebsd.org> Subject: Re: svn commit: r202889 - head/sys/kern Message-ID: <3bbf2fe11001260339u7a694069m6a2bb7e18b2c546a@mail.gmail.com> In-Reply-To: <3bbf2fe11001260058i65604619l664bd0e49c1dbbd@mail.gmail.com> References: <201001231554.o0NFsMbx049837@svn.freebsd.org> <201001251456.32459.jhb@freebsd.org> <3bbf2fe11001260058i65604619l664bd0e49c1dbbd@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2010/1/26 Attilio Rao <attilio@freebsd.org>: > 2010/1/25 John Baldwin <jhb@freebsd.org>: >> On Saturday 23 January 2010 10:54:22 am Attilio Rao wrote: >>> Author: attilio >>> Date: Sat Jan 23 15:54:21 2010 >>> New Revision: 202889 >>> URL: http://svn.freebsd.org/changeset/base/202889 >>> >>> Log: >>> =C2=A0 - Fix a race in sched_switch() of sched_4bsd. >>> =C2=A0 =C2=A0 In the case of the thread being on a sleepqueue or a turn= stile, the >>> =C2=A0 =C2=A0 sched_lock was acquired (without the aid of the td_lock i= nterface) and >>> =C2=A0 =C2=A0 the td_lock was dropped. This was going to break locking = rules on other >>> =C2=A0 =C2=A0 threads willing to access to the thread (via the td_lock = interface) and >>> =C2=A0 =C2=A0 modify his flags (allowed as long as the container lock w= as different >>> =C2=A0 =C2=A0 by the one used in sched_switch). >>> =C2=A0 =C2=A0 In order to prevent this situation, while sched_lock is a= cquired there >>> =C2=A0 =C2=A0 the td_lock gets blocked. [0] >>> =C2=A0 - Merge the ULE's internal function thread_block_switch() into t= he global >>> =C2=A0 =C2=A0 thread_lock_block() and make the former semantic as the d= efault for >>> =C2=A0 =C2=A0 thread_lock_block(). This means that thread_lock_block() = will not >>> =C2=A0 =C2=A0 disable interrupts when called (and consequently thread_u= nlock_block() >>> =C2=A0 =C2=A0 will not re-enabled them when called). This should be don= e manually >>> =C2=A0 =C2=A0 when necessary. >>> =C2=A0 =C2=A0 Note, however, that ULE's thread_unblock_switch() is not = reaped >>> =C2=A0 =C2=A0 because it does reflect a difference in semantic due in U= LE (the >>> =C2=A0 =C2=A0 td_lock may not be necessarilly still blocked_lock when c= alling this). >>> =C2=A0 =C2=A0 While asymmetric, it does describe a remarkable differenc= e in semantic >>> =C2=A0 =C2=A0 that is good to keep in mind. >> >> Does this affect the various #ifdef's for handling the third argument to >> cpu_switch()? =C2=A0E.g. does 4BSD need to spin if td_lock is &blocked_l= ock? >> >> Also, BLOCK_SPIN() on x86 is non-optimal. =C2=A0It should not do cmpxchg= in a loop. >> Instead, it should do cmp in a loop, and if the cmp succeeds, then try >> cmpxchg. > > [CC'ing imp@ because he made a similar question privately] > > Basically it is recounduited to the difference, in terms of runqueue > between sched_4bsd and sched_ule. > sched_ule has one runqueue per-core and sched_4bsd has just one runqueue. > The codepath you are referring to, for the ULE case, is used when > threads need to migrate from one cpu to another. > What should really happen is that threads should be both locked (thus > the runqueues where the in & out threads are present should be both > locked) but it will create cyclic LOR and then a 'blocked_lock' is > used (in order to avoid such LORs). > When you are switching in a thread caming from another CPU you need to > make sure its lock already changed to, possibly, the new runqueue one > (by the CPU where it *was* running that is going to switch it out). > That happens because we have not a global interlock between the CPUs > and thread migration gets a bit trickier. > > Said that, the !SMP case just assume there is one runqueue, so this > discussion is not useful (you won't have different runqueues from > where the thread may be caming, but just one protected by sched_lock). > Also sched_4bsd has just one runqueue, thus this discussion should not > apply there. > > What the last 4bsd patch does is to keep track of the 'old' > thread_lock and assign it to the switched out thread, which may now be > blocked, when it is safe to do that (after the tlb shootdown in > ia32/amd64 cpu_switch() case). > However, this may generate a bit of confusion, because 'blocked' has a > different meaning between the 2 cases: > * ULE does that in order to prevent LOR between runqueues CPUs. > * 4BSD now does that in order to be able to not end with 2 spinlocks > held in some cases (note: this is not a LOR because there is the well > defined order of sq_lock/ts_lock -> sched_lock) but it makes > cpu_switch() simpler, faster and reduces contention. > It is enough that cpu_switch() does handle the third argument properly > and sets back the old lock when it is safe. > I see, however, that this may be a problem for architectures that were > just supporting 4BSD and not ULE (at least to some extent), like in > the sparc64 case. If an architecture does not ignore the third > argument of cpu_switch() and does set it properly there should be a > problem with sched_4bsd. I should probabilly poke the maintainers of > any arch and check with them if there are further problems. I checked for available architectures and I think that sparc64 (and possibly sun4v) is the only one with missing bits. > Finally, I really don't think the BLOCK_SPIN() performance improvement > you suggest should really make a difference (assuming how rarely it > should happen) because, please note, that the cmpxchg is necessary as > we need to enforce a memory barrier when doing the check in anyway (so > the first case should be however an atomic_cmpset_X_Y()). I think that ia64 is broken on that regard because it does use simple operation while it should use correct atomic and memory barriers (CC'ed marcel@ for that). Thanks, Attilio --=20 Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3bbf2fe11001260339u7a694069m6a2bb7e18b2c546a>