From owner-freebsd-hackers@FreeBSD.ORG Sun Aug 8 15:24:39 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 3A6B81065673; Sun, 8 Aug 2010 15:24:39 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-iw0-f182.google.com (mail-iw0-f182.google.com [209.85.214.182]) by mx1.freebsd.org (Postfix) with ESMTP id E2D778FC13; Sun, 8 Aug 2010 15:24:38 +0000 (UTC) Received: by iwn10 with SMTP id 10so3791883iwn.13 for ; Sun, 08 Aug 2010 08:24:38 -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=k1e/gT/VQjzHm52ozxtyP0vSLLrACCcOZfwuaBr7SC4=; b=m+8RwkMsAm6DozluQe3Opks6glAdEF64TiZOTizq5YA6G4wYP2fBDr7RKshQNUYrCo J3C+V0XOM3Tdzq8o8iOzPsl0ffAuCDra0qc4gRZZul8yGhzlWDIQxTWiGbC+YiL8oqj1 IaYvQ3OjGd9TDpX3Euuz7Urvv3/J6QF4RURbE= 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=YOuUs6IYI5STliEhAt/Bh67mPNzl4BgABMva1X6As2AcBxBL6yp1sX67nCezGkuFpk Erfgnb0O+2BceoI0Un22vBLIL6YV/Nt5eWAmplsDlnI3oRgWFcWpmmV/tGFn3w6Khpln WA5d+vjA1fNsUP/dTW2ztolonGRp16myxiyj4= MIME-Version: 1.0 Received: by 10.42.0.195 with SMTP id 3mr4672738icd.83.1281281078073; Sun, 08 Aug 2010 08:24:38 -0700 (PDT) Sender: mdf356@gmail.com Received: by 10.42.3.140 with HTTP; Sun, 8 Aug 2010 08:24:38 -0700 (PDT) In-Reply-To: References: <201007301008.22501.jhb@freebsd.org> <201007301031.34266.jhb@freebsd.org> Date: Sun, 8 Aug 2010 15:24:38 +0000 X-Google-Sender-Auth: 0oIbECYfW3kN6qjoZfH2W54G1n4 Message-ID: From: mdf@FreeBSD.org To: Attilio Rao Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: 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: Sun, 08 Aug 2010 15:24:39 -0000 On Sun, Aug 8, 2010 at 2:43 PM, Attilio Rao wrote: > 2010/8/4 =A0: >> On Fri, Jul 30, 2010 at 2:31 PM, John Baldwin wrote: >>> 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 ast() >>>> > indicates the sched lock is still held, but the place it claims it w= as >>>> > held by is in fact sometimes not possible to keep the lock, like: >>>> > >>>> > =A0 =A0 thread_lock(td); >>>> > =A0 =A0 td->td_flags &=3D ~TDF_SELECT; >>>> > =A0 =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: =A0 65 48 8b 1c 25 00 00 =A0 =A0mov =A0 =A0%gs:0x0= ,%rbx >>>> > ffffffff802db217: =A0 00 00 >>>> > ffffffff802db219: =A0 ff 83 04 01 00 00 =A0 =A0 =A0 incl =A0 0x104(%= rbx) >>>> > =A0 =A0 =A0* Pin the thread in order to avoid problems with thread m= igration. >>>> > =A0 =A0 =A0* Once that all verifies are passed about spinlocks owner= ship, >>>> > =A0 =A0 =A0* the thread is in a safe path and it can be unpinned. >>>> > =A0 =A0 =A0*/ >>>> > =A0 =A0 sched_pin(); >>>> > =A0 =A0 lock_list =3D PCPU_GET(spinlocks); >>>> > ffffffff802db21f: =A0 65 48 8b 04 25 48 00 =A0 =A0mov =A0 =A0%gs:0x4= 8,%rax >>>> > ffffffff802db226: =A0 00 00 >>>> > =A0 =A0 if (lock_list !=3D NULL && lock_list->ll_count !=3D 0) { >>>> > ffffffff802db228: =A0 48 85 c0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0test = =A0 %rax,%rax >>>> > =A0 =A0 =A0* Pin the thread in order to avoid problems with thread m= igration. >>>> > =A0 =A0 =A0* Once that all verifies are passed about spinlocks owner= ship, >>>> > =A0 =A0 =A0* the thread is in a safe path and it can be unpinned. >>>> > =A0 =A0 =A0*/ >>>> > =A0 =A0 sched_pin(); >>>> > =A0 =A0 lock_list =3D PCPU_GET(spinlocks); >>>> > ffffffff802db22b: =A0 48 89 85 f0 fe ff ff =A0 =A0mov =A0 =A0%rax,-0= x110(%rbp) >>>> > ffffffff802db232: =A0 48 89 85 f8 fe ff ff =A0 =A0mov =A0 =A0%rax,-0= x108(%rbp) >>>> > =A0 =A0 if (lock_list !=3D NULL && lock_list->ll_count !=3D 0) { >>>> > ffffffff802db239: =A0 0f 84 ff 00 00 00 =A0 =A0 =A0 je =A0 =A0 fffff= fff802db33e >>>> > >>>> > ffffffff802db23f: =A0 44 8b 60 50 =A0 =A0 =A0 =A0 =A0 =A0 mov =A0 = =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 have a >>>> > lock leak at the indicated point, but in one instance I can see in t= he >>>> > dump that the lock_list local from witness_warn is from the pcpu >>>> > structure for CPU 0 (and I was warned about sched lock 0), but the >>>> > thread id in panic_cpu is 2. =A0So clearly the thread was being migr= ated >>>> > right around panic time. >>>> > >>>> > This is the amd64 kernel on stable/7. =A0I'm not sure exactly what k= ind >>>> > of hardware; it's a 4-way Intel chip from about 3 or 4 years ago IIR= C. >>>> > >>>> > So... do we need some kind of barrier in the code for sched_pin() fo= r >>>> > it to really do what it claims? =A0Could the hardware have re-ordere= d >>>> > the "mov =A0 =A0%gs:0x48,%rax" PCPU_GET to before the sched_pin() >>>> > increment? >>>> >>>> Hmmm, I think it might be able to because they refer to different loca= tions. >>>> >>>> Note this rule in section 8.2.2 of Volume 3A: >>>> >>>> =A0 =95 Reads may be reordered with older writes to different location= s but not >>>> =A0 =A0 with older writes to the same location. >>>> >>>> It is certainly true that sparc64 could reorder with RMO. =A0I believe= ia64 >>>> could reorder as well. =A0Since sched_pin/unpin are frequently used to= provide >>>> this sort of synchronization, we could use memory barriers in pin/unpi= n >>>> like so: >>>> >>>> sched_pin() >>>> { >>>> =A0 =A0 =A0 td->td_pinned =3D atomic_load_acq_int(&td->td_pinned) + 1; >>>> } >>>> >>>> sched_unpin() >>>> { >>>> =A0 =A0 =A0 atomic_store_rel_int(&td->td_pinned, td->td_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 what is h= appening >>>> I think. >>> >>> However, to actually get a race you'd have to have an interrupt fire an= d >>> migrate you so that the speculative read was from the other CPU. =A0How= ever, I >>> don't think the speculative read would be preserved in that case. =A0Th= e CPU >>> has to return to a specific PC when it returns from the interrupt and i= t has >>> no way of storing the state for what speculative reordering it might be >>> doing, so presumably it is thrown away? =A0I suppose it is possible tha= t it >>> actually retires both instructions (but reordered) and then returns to = the PC >>> value after the read of listlocks after the interrupt. =A0However, in t= hat case >>> the scheduler would not migrate as it would see td_pinned !=3D 0. =A0To= get the >>> race you have to have the interrupt take effect prior to modifying td_p= inned, >>> so I think the processor would have to discard the reordered read 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 doi= ng >>> both a read and a write and another rule in the section 8.2.2 is this: >>> >>> =A0=95 Reads are not reordered with other reads. >>> >>> That would seem to prevent the read of listlocks from passing the read = of >>> td_pinned in the incl instruction on x86. >> >> I wonder how that's interpreted in the microcode, though? =A0I.e. if the >> incr instruction decodes to load, add, store, does the h/w allow the >> later reads to pass the final store? >> >> I added the following: >> >> =A0 =A0 =A0 =A0sched_pin(); >> =A0 =A0 =A0 =A0lock_list =3D PCPU_GET(spinlocks); >> =A0 =A0 =A0 =A0if (lock_list !=3D NULL && lock_list->ll_count !=3D 0) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* XXX debug for bug 67957 */ >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mfence(); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 lle =3D PCPU_GET(spinlocks); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (lle !=3D lock_list) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 panic("Bug 67957: had lock= list %p, now %p\n", >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 lock_list, lle); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* XXX end debug */ >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sched_unpin(); >> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* >> >> ... and the panic triggered. =A0I think it's more likely that some >> barrier is needed in sched_pin() than that %gs is getting corrupted >> but can always be dereferenced. > > Are the 2 values just different or one of the 2 is NULL? They are just different. I don't have a valid dump for this panic, but the earlier one before I added this check showed that the two are likely to be from different PCPU structs. Thanks, matthew