From owner-freebsd-hackers@FreeBSD.ORG Fri Aug 6 23:28:41 2010 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0E6CA106566B; Fri, 6 Aug 2010 23:28:41 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-ww0-f42.google.com (mail-ww0-f42.google.com [74.125.82.42]) by mx1.freebsd.org (Postfix) with ESMTP id 420988FC08; Fri, 6 Aug 2010 23:28:39 +0000 (UTC) Received: by wwf26 with SMTP id 26so383589wwf.1 for ; Fri, 06 Aug 2010 16:28:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:sender:received :in-reply-to:references:date:x-google-sender-auth:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=PhPny5An+nrksa7qkUH1e+mZc+KwFCupwJMnw2Z8bXY=; b=RzB9qUpZM7vTj/0dgX6cK0H2vO+XZewoWfJKGwoORVFmnn7XEZYFt8ldnm6Eb8VbDJ HN6YN/ZB/CiLo4skMqCQ0EgR89GRtNUujCoFfitOVJLjfXR/pbvbh2BnSH/mGtlbkv6G n6/xkplzPdh6i5vU6hQdik1F7+U5DkNkPThhk= 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=eCCjq90Zi1tECDuLr2kaGk6gI81v2GHWhbEXzXWWis9lw3/YHPgUNp6CyhI3MgSZRM T98IWV0vR/7mRn7XnMjsH0ZT7hkx5WWapau0umwTDRl7LwunuzsVqqgj2/qKhY9WPTjN v99FvyhzmJqbNMmUH9HuOH2QAO2y9OHbtgTcc= MIME-Version: 1.0 Received: by 10.216.79.66 with SMTP id h44mr11215225wee.6.1281137318946; Fri, 06 Aug 2010 16:28:38 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.216.238.16 with HTTP; Fri, 6 Aug 2010 16:28:38 -0700 (PDT) In-Reply-To: <201008051312.25854.jhb@freebsd.org> References: <201008041455.26066.jhb@freebsd.org> <201008051312.25854.jhb@freebsd.org> Date: Sat, 7 Aug 2010 01:28:38 +0200 X-Google-Sender-Auth: e7TvOTDuU9b0EpaEK-PxJ_S-Bak Message-ID: From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: mdf@freebsd.org, freebsd-hackers@freebsd.org Subject: Re: sched_pin() versus PCPU_GET X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 Aug 2010 23:28:41 -0000 2010/8/5 John Baldwin : > On Thursday, August 05, 2010 11:59:37 am mdf@freebsd.org wrote: >> On Wed, Aug 4, 2010 at 11:55 AM, John Baldwin wrote: >> > On Wednesday, August 04, 2010 12:20:31 pm mdf@freebsd.org wrote: >> >> On Wed, Aug 4, 2010 at 2:26 PM, John Baldwin wrote: >> >> > On Tuesday, August 03, 2010 9:46:16 pm mdf@freebsd.org wrote: >> >> >> On Fri, Jul 30, 2010 at 2:31 PM, John Baldwin wr= ote: >> >> >> > On Friday, July 30, 2010 10:08:22 am John Baldwin wrote: >> >> >> >> On Thursday, July 29, 2010 7:39:02 pm mdf@freebsd.org wrote: >> >> >> >> > We've seen a few instances at work where witness_warn() in as= t() >> >> >> >> > indicates the sched lock is still held, but the place it clai= ms > it was >> >> >> >> > held by is in fact sometimes not possible to keep the lock, l= ike: >> >> >> >> > >> >> >> >> > =C2=A0 =C2=A0 thread_lock(td); >> >> >> >> > =C2=A0 =C2=A0 td->td_flags &=3D ~TDF_SELECT; >> >> >> >> > =C2=A0 =C2=A0 thread_unlock(td); >> >> >> >> > >> >> >> >> > What I was wondering is, even though the assembly I see in > objdump -S >> >> >> >> > for witness_warn has the increment of td_pinned before the > PCPU_GET: >> >> >> >> > >> >> >> >> > ffffffff802db210: =C2=A0 65 48 8b 1c 25 00 00 =C2=A0 =C2=A0mo= v =C2=A0 =C2=A0%gs:0x0,%rbx >> >> >> >> > ffffffff802db217: =C2=A0 00 00 >> >> >> >> > ffffffff802db219: =C2=A0 ff 83 04 01 00 00 =C2=A0 =C2=A0 =C2= =A0 incl =C2=A0 0x104(%rbx) >> >> >> >> > =C2=A0 =C2=A0 =C2=A0* Pin the thread in order to avoid proble= ms with thread > migration. >> >> >> >> > =C2=A0 =C2=A0 =C2=A0* Once that all verifies are passed about= spinlocks > ownership, >> >> >> >> > =C2=A0 =C2=A0 =C2=A0* the thread is in a safe path and it can= be unpinned. >> >> >> >> > =C2=A0 =C2=A0 =C2=A0*/ >> >> >> >> > =C2=A0 =C2=A0 sched_pin(); >> >> >> >> > =C2=A0 =C2=A0 lock_list =3D PCPU_GET(spinlocks); >> >> >> >> > ffffffff802db21f: =C2=A0 65 48 8b 04 25 48 00 =C2=A0 =C2=A0mo= v =C2=A0 =C2=A0%gs:0x48,%rax >> >> >> >> > ffffffff802db226: =C2=A0 00 00 >> >> >> >> > =C2=A0 =C2=A0 if (lock_list !=3D NULL && lock_list->ll_count = !=3D 0) { >> >> >> >> > ffffffff802db228: =C2=A0 48 85 c0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0test =C2=A0 %rax,%rax >> >> >> >> > =C2=A0 =C2=A0 =C2=A0* Pin the thread in order to avoid proble= ms with thread > migration. >> >> >> >> > =C2=A0 =C2=A0 =C2=A0* Once that all verifies are passed about= spinlocks > ownership, >> >> >> >> > =C2=A0 =C2=A0 =C2=A0* the thread is in a safe path and it can= be unpinned. >> >> >> >> > =C2=A0 =C2=A0 =C2=A0*/ >> >> >> >> > =C2=A0 =C2=A0 sched_pin(); >> >> >> >> > =C2=A0 =C2=A0 lock_list =3D PCPU_GET(spinlocks); >> >> >> >> > ffffffff802db22b: =C2=A0 48 89 85 f0 fe ff ff =C2=A0 =C2=A0mo= v > =C2=A0%rax,-0x110(%rbp) >> >> >> >> > ffffffff802db232: =C2=A0 48 89 85 f8 fe ff ff =C2=A0 =C2=A0mo= v > =C2=A0%rax,-0x108(%rbp) >> >> >> >> > =C2=A0 =C2=A0 if (lock_list !=3D NULL && lock_list->ll_count = !=3D 0) { >> >> >> >> > ffffffff802db239: =C2=A0 0f 84 ff 00 00 00 =C2=A0 =C2=A0 =C2= =A0 je > ffffffff802db33e >> >> >> >> > >> >> >> >> > ffffffff802db23f: =C2=A0 44 8b 60 50 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 mov =C2=A0 =C2=A00x50(%rax), > %r12d >> >> >> >> > >> >> >> >> > is it possible for the hardware to do any re-ordering here? >> >> >> >> > >> >> >> >> > The reason I'm suspicious is not just that the code doesn't h= ave > a >> >> >> >> > lock leak at the indicated point, but in one instance I can s= ee > in the >> >> >> >> > dump that the lock_list local from witness_warn is from the p= cpu >> >> >> >> > structure for CPU 0 (and I was warned about sched lock 0), bu= t > the >> >> >> >> > thread id in panic_cpu is 2. =C2=A0So clearly the thread was = being > migrated >> >> >> >> > right around panic time. >> >> >> >> > >> >> >> >> > This is the amd64 kernel on stable/7. =C2=A0I'm not sure exac= tly what > kind >> >> >> >> > of hardware; it's a 4-way Intel chip from about 3 or 4 years = ago > IIRC. >> >> >> >> > >> >> >> >> > So... do we need some kind of barrier in the code for sched_p= in() > for >> >> >> >> > it to really do what it claims? =C2=A0Could the hardware have= re- > ordered >> >> >> >> > the "mov =C2=A0 =C2=A0%gs:0x48,%rax" PCPU_GET to before the s= ched_pin() >> >> >> >> > increment? >> >> >> >> >> >> >> >> Hmmm, I think it might be able to because they refer to differe= nt > locations. >> >> >> >> >> >> >> >> Note this rule in section 8.2.2 of Volume 3A: >> >> >> >> >> >> >> >> =C2=A0 =E2=80=A2 Reads may be reordered with older writes to di= fferent locations > but not >> >> >> >> =C2=A0 =C2=A0 with older writes to the same location. >> >> >> >> >> >> >> >> It is certainly true that sparc64 could reorder with RMO. =C2= =A0I > believe ia64 >> >> >> >> could reorder as well. =C2=A0Since sched_pin/unpin are frequent= ly used > to provide >> >> >> >> this sort of synchronization, we could use memory barriers in > pin/unpin >> >> >> >> like so: >> >> >> >> >> >> >> >> sched_pin() >> >> >> >> { >> >> >> >> =C2=A0 =C2=A0 =C2=A0 td->td_pinned =3D atomic_load_acq_int(&td-= >td_pinned) + 1; >> >> >> >> } >> >> >> >> >> >> >> >> sched_unpin() >> >> >> >> { >> >> >> >> =C2=A0 =C2=A0 =C2=A0 atomic_store_rel_int(&td->td_pinned, td->t= d_pinned - 1); >> >> >> >> } >> >> >> >> >> >> >> >> We could also just use atomic_add_acq_int() and > atomic_sub_rel_int(), but they >> >> >> >> are slightly more heavyweight, though it would be more clear wh= at > is happening >> >> >> >> I think. >> >> >> > >> >> >> > However, to actually get a race you'd have to have an interrupt = fire > and >> >> >> > migrate you so that the speculative read was from the other CPU. > =C2=A0However, I >> >> >> > don't think the speculative read would be preserved in that case= . > =C2=A0The CPU >> >> >> > has to return to a specific PC when it returns from the interrup= t > and it has >> >> >> > no way of storing the state for what speculative reordering it m= ight > be >> >> >> > doing, so presumably it is thrown away? =C2=A0I suppose it is po= ssible > that it >> >> >> > actually retires both instructions (but reordered) and then retu= rns > to the PC >> >> >> > value after the read of listlocks after the interrupt. =C2=A0How= ever, in > that case >> >> >> > the scheduler would not migrate as it would see td_pinned !=3D 0= . =C2=A0To > get the >> >> >> > race you have to have the interrupt take effect prior to modifyi= ng > td_pinned, >> >> >> > so I think the processor would have to discard the reordered rea= d of >> >> >> > listlocks so it could safely resume execution at the 'incl' > instruction. >> >> >> > >> >> >> > The other nit there on x86 at least is that the incl instruction= is > doing >> >> >> > both a read and a write and another rule in the section 8.2.2 is > this: >> >> >> > >> >> >> > =C2=A0=E2=80=A2 Reads are not reordered with other reads. >> >> >> > >> >> >> > That would seem to prevent the read of listlocks from passing th= e > read of >> >> >> > td_pinned in the incl instruction on x86. >> >> >> >> >> >> I wonder how that's interpreted in the microcode, though? =C2=A0I.= e. if the >> >> >> incr instruction decodes to load, add, store, does the h/w allow t= he >> >> >> later reads to pass the final store? >> >> > >> >> > Well, the architecture is defined in terms of the ISA, not the > microcode, per >> >> > se, so I think it would have to treat the read for the incl as bein= g an > earlier >> >> > read than 'spinlocks'. >> >> > >> >> >> I added the following: >> >> >> >> >> >> =C2=A0 =C2=A0 =C2=A0 sched_pin(); >> >> >> =C2=A0 =C2=A0 =C2=A0 lock_list =3D PCPU_GET(spinlocks); >> >> >> =C2=A0 =C2=A0 =C2=A0 if (lock_list !=3D NULL && lock_list->ll_coun= t !=3D 0) { >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* XXX debug for bug 6= 7957 */ >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mfence(); >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 lle =3D PCPU_GET(spinl= ocks); >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (lle !=3D lock_list= ) { >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 panic("Bug 67957: had lock list %p, now %p\n", >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 lock_list, lle); >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 } >> >> >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* XXX end debug */ >> >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sched_unpin(); >> >> >> >> >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* >> >> >> >> >> >> ... and the panic triggered. =C2=A0I think it's more likely that s= ome >> >> >> barrier is needed in sched_pin() than that %gs is getting corrupte= d >> >> >> but can always be dereferenced. >> >> > >> >> > Actually, I would beg to differ in that case. =C2=A0If PCPU_GET(spi= nlocks) >> >> > returns non-NULL, then it means that you hold a spin lock, >> >> >> >> ll_count is 0 for the "correct" pc_spinlocks and non-zero for the >> >> "wrong" one, though. =C2=A0So I think it can be non-NULL but the curr= ent >> >> thread/CPU doesn't hold a spinlock. >> > >> > Hmm, does the 'lock_list' pointer value in the dump match 'lock_list' >> > from another CPU? >> >> Yes: >> >> (gdb) p panic_cpu >> $9 =3D 2 >> (gdb) p dumptid >> $12 =3D 100751 >> (gdb) p cpuhead.slh_first->pc_allcpu.sle_next->pc_curthread->td_tid >> $14 =3D 100751 >> >> (gdb) p *cpuhead.slh_first->pc_allcpu.sle_next >> $6 =3D { >> =C2=A0 pc_curthread =3D 0xffffff00716d6960, >> =C2=A0 pc_cpuid =3D 2, >> =C2=A0 pc_spinlocks =3D 0xffffffff80803198, >> >> (gdb) p lock_list >> $2 =3D (struct lock_list_entry *) 0xffffffff80803fb0 >> >> (gdb) p *cpuhead.slh_first->pc_allcpu.sle_next->pc_allcpu.sle_next- >>pc_allcpu.sle_next >> $8 =3D { >> =C2=A0 pc_curthread =3D 0xffffff0005479960, >> =C2=A0 pc_cpuid =3D 0, >> =C2=A0 pc_spinlocks =3D 0xffffffff80803fb0, >> >> I.e. we're dumping on CPU 2, but the lock_list pointer that was saved >> in the dump matches that of CPU 0. > > Can you print out the tid's for the two curthreads? =C2=A0It's not imposs= ible that > the thread migrated after calling panic. =C2=A0In fact we force threads t= o CPU 0 > during shutdown. The problem here is not migration but the fact that witness thinks there is a spinlock held when it is not true (we are sure the code is not bogus, right?). As long as a thread staying on a cpu has a spinlock it can't be switched out, we should rule out any migration specific problem and the only way the first thing that cames to my mind is a stale %gs. Thanks, Attilio --=20 Peace can only be achieved by understanding - A. Einstein