From owner-svn-src-all@FreeBSD.ORG Tue Jan 26 11:39:06 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 360891065672; Tue, 26 Jan 2010 11:39:06 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-iw0-f204.google.com (mail-iw0-f204.google.com [209.85.223.204]) by mx1.freebsd.org (Postfix) with ESMTP id AA80F8FC15; Tue, 26 Jan 2010 11:39:05 +0000 (UTC) Received: by iwn42 with SMTP id 42so1475746iwn.9 for ; Tue, 26 Jan 2010 03:39:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type:content-transfer-encoding; bh=UfhGByoSpds4w3m/WnTXFRWi+llFW2LnHkVUfZf3VUM=; b=ktxWhwMKIkbPMIn69kqbPCLas0+59fdXOE7TVkvdDLaHJtG1q1ZTgGgwBxk6s31u24 EaH1qijBl7URJx/wzdEj8Lr5j/9A2osQO4ZoEMCZ7XtQgInaflIWzui7lmAGMuoeHT3z VFOg0U+sHb4QFhdrRHEvbFzvAIjOBeqsAalVE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; b=MIvEWhI0tAiKFMs/ThHqLN6psvk3MPlbiPrndufupGz+yQoC7vOdc2dRTceiamRKmh ODQmifCZ2/hdr9IUjEmrbFukVYNq9AiCeVO1os8gQyzM2mb+dBEQGT6ljpoj1M5RI/Nl b49Vz96QwqjuwoJKZCc81CECOpCDB/GMbcSJY= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.231.146.134 with SMTP id h6mr866263ibv.16.1264505942231; Tue, 26 Jan 2010 03:39:02 -0800 (PST) In-Reply-To: <3bbf2fe11001260058i65604619l664bd0e49c1dbbd@mail.gmail.com> References: <201001231554.o0NFsMbx049837@svn.freebsd.org> <201001251456.32459.jhb@freebsd.org> <3bbf2fe11001260058i65604619l664bd0e49c1dbbd@mail.gmail.com> Date: Tue, 26 Jan 2010 12:39:02 +0100 X-Google-Sender-Auth: a4bfe94f10e65e70 Message-ID: <3bbf2fe11001260339u7a694069m6a2bb7e18b2c546a@mail.gmail.com> From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Warner Losh , Marcel Moolenaar Subject: Re: svn commit: r202889 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jan 2010 11:39:06 -0000 2010/1/26 Attilio Rao : > 2010/1/25 John Baldwin : >> 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