From owner-freebsd-hackers@FreeBSD.ORG Wed Aug 4 16:20:33 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 259BC106566C; Wed, 4 Aug 2010 16:20:33 +0000 (UTC) (envelope-from mdf356@gmail.com) Received: from mail-pv0-f182.google.com (mail-pv0-f182.google.com [74.125.83.182]) by mx1.freebsd.org (Postfix) with ESMTP id ACA808FC12; Wed, 4 Aug 2010 16:20:32 +0000 (UTC) Received: by pvh1 with SMTP id 1so2306099pvh.13 for ; Wed, 04 Aug 2010 09:20:32 -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=Yem72Wg8t2zOSB7igFgTlmV3HZjhrDx5hgzYXguLuGU=; b=pv0JQCzStY1s6hwNdbDTlZuOWM3S1blpc7HaVcNEyxaMl8/7vMFrIYwryOkCW00T1V xxD1rWX3/AqxlJSMmfRW8yXC8ZGgkQL97o/PMqJlY57zslZ+kstVdXZRBZQNWd2GJJWn JAVNmqAAxzzsitjLechujUULjPsynLX4oHD24= 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=VcR53/t19FouyAekce8EX07rkq2Byvl3PV6j7qPx6Z5NXw9eGifacDihXatDlIkdZ1 7Pn7Q8qW+VITq17RLY1MdDbohhrKP+sSawepfvquOtqrUTDM1ZIQhtB1I1D3bSWisdsn ZwVDaUBCWClPup1F27m10BuXUPHUpIprXj+WE= MIME-Version: 1.0 Received: by 10.142.133.20 with SMTP id g20mr7990320wfd.0.1280938832244; Wed, 04 Aug 2010 09:20:32 -0700 (PDT) Sender: mdf356@gmail.com Received: by 10.42.6.85 with HTTP; Wed, 4 Aug 2010 09:20:31 -0700 (PDT) In-Reply-To: <201008041026.17553.jhb@freebsd.org> References: <201007301031.34266.jhb@freebsd.org> <201008041026.17553.jhb@freebsd.org> Date: Wed, 4 Aug 2010 16:20:31 +0000 X-Google-Sender-Auth: MszHhj851rfOFBM2bmCV5HPhoJE Message-ID: From: mdf@FreeBSD.org To: John Baldwin 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: Wed, 04 Aug 2010 16:20:33 -0000 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 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 = was >> >> > 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:0x= 0,%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 = migration. >> >> > =A0 =A0 =A0* Once that all verifies are passed about spinlocks owne= rship, >> >> > =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:0x= 48,%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 = migration. >> >> > =A0 =A0 =A0* Once that all verifies are passed about spinlocks owne= rship, >> >> > =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,-= 0x110(%rbp) >> >> > ffffffff802db232: =A0 48 89 85 f8 fe ff ff =A0 =A0mov =A0 =A0%rax,-= 0x108(%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 ffff= ffff802db33e >> >> > >> >> > 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 = the >> >> > 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 mig= rated >> >> > right around panic time. >> >> > >> >> > This is the amd64 kernel on stable/7. =A0I'm not sure exactly what = kind >> >> > of hardware; it's a 4-way Intel chip from about 3 or 4 years ago II= RC. >> >> > >> >> > So... do we need some kind of barrier in the code for sched_pin() f= or >> >> > it to really do what it claims? =A0Could the hardware have re-order= ed >> >> > 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 loc= ations. >> >> >> >> Note this rule in section 8.2.2 of Volume 3A: >> >> >> >> =A0 =95 Reads may be reordered with older writes to different locatio= ns but not >> >> =A0 =A0 with older writes to the same location. >> >> >> >> It is certainly true that sparc64 could reorder with RMO. =A0I believ= e ia64 >> >> could reorder as well. =A0Since sched_pin/unpin are frequently used t= o provide >> >> this sort of synchronization, we could use memory barriers in pin/unp= in >> >> 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 = happening >> >> I think. >> > >> > However, to actually get a race you'd have to have an interrupt fire a= nd >> > migrate you so that the speculative read was from the other CPU. =A0Ho= wever, I >> > don't think the speculative read would be preserved in that case. =A0T= he CPU >> > has to return to a specific PC when it returns from the interrupt and = it has >> > no way of storing the state for what speculative reordering it might b= e >> > doing, so presumably it is thrown away? =A0I suppose it is possible th= at it >> > actually retires both instructions (but reordered) and then returns to= the PC >> > value after the read of listlocks after the interrupt. =A0However, in = that case >> > the scheduler would not migrate as it would see td_pinned !=3D 0. =A0T= o get the >> > race you have to have the interrupt take effect prior to modifying td_= pinned, >> > so I think the processor would have to discard the reordered read of >> > listlocks so it could safely resume execution at the 'incl' instructio= n. >> > >> > The other nit there on x86 at least is that the incl instruction is do= ing >> > 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? > > 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 being an e= arlier > read than 'spinlocks'. > >> I added the following: >> >> =A0 =A0 =A0 sched_pin(); >> =A0 =A0 =A0 lock_list =3D PCPU_GET(spinlocks); >> =A0 =A0 =A0 if (lock_list !=3D NULL && lock_list->ll_count !=3D 0) { >> + =A0 =A0 =A0 =A0 =A0 =A0 /* XXX debug for bug 67957 */ >> + =A0 =A0 =A0 =A0 =A0 =A0 mfence(); >> + =A0 =A0 =A0 =A0 =A0 =A0 lle =3D PCPU_GET(spinlocks); >> + =A0 =A0 =A0 =A0 =A0 =A0 if (lle !=3D lock_list) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 panic("Bug 67957: had lock lis= t %p, now %p\n", >> + =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 /* XXX end debug */ >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 sched_unpin(); >> >> =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. > > Actually, I would beg to differ in that case. =A0If PCPU_GET(spinlocks) > 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. So I think it can be non-NULL but the current thread/CPU doesn't hold a spinlock. I don't believe we have any code in the NMI handler. I'm on vacation today so I'll check tomorrow. Thanks, matthew > which > means that interrupts are disabled and have been disabled for "a while" > (from when the spin lock was acquired prior trying to acquire the > lock that you hold now). =A0I think that means that the only way you can > have a problem is if you get an NMI. =A0Do you have custom logic in your > NMI handler? =A0Perhaps it isn't calling swapgs correctly (either when it > shouldn't, or isn't calling it when it should). =A0I know that the Joseph > Koshy spent a good bit of time getting the %gs handling in the NMI > handler correct to handle NMIs firing at "bad" times (such as during the > very end of a syscall return). > > It is worth noting that when the spinlock was acquired "ages" ago, it > did a critical_enter() which would have forbid any context switches, > so the thread would not have migrated to another CPU. =A0I think it is > almost certainly a corrupt %gs. > >> An mfence() at the end of sched_pin() would be sufficient, but it >> seems like overkill since all we really need is to prevent instruction >> re-ordering. =A0As I said above, on PowerPC this would be isync; what is >> the equivalent on x86? =A0I can try it out and see if this panic goes >> away. > > mfence() is the closest thing x86 has. =A0It does not have an equivalent > to isync as it is still defined to complete instructions in "program > order". =A0It does not reorder instructions, merely the scheduling of > memory transactions from what I can tell. =A0Mostly I think that all this > is just to allow it to store writes in its store buffer, but a CPU will > snoop its own store buffer to satisfy reads IIRC, so by and large a CPU > is consistent with itself. > > -- > John Baldwin >