Date: Wed, 4 Aug 2010 16:20:31 +0000 From: mdf@FreeBSD.org To: John Baldwin <jhb@freebsd.org> Cc: freebsd-hackers@freebsd.org Subject: Re: sched_pin() versus PCPU_GET Message-ID: <AANLkTi=YjbBdZp9KuGUmMuYUmWyx_n%2BykikPSMMMo=j9@mail.gmail.com> In-Reply-To: <201008041026.17553.jhb@freebsd.org> References: <AANLkTikY20TxyeyqO5zP3zC-azb748kV-MdevPfm%2B8cq@mail.gmail.com> <201007301031.34266.jhb@freebsd.org> <AANLkTinp7278ZD1L8s616seQET=OQBx1RZ4eHx=e%2BpD5@mail.gmail.com> <201008041026.17553.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Aug 4, 2010 at 2:26 PM, John Baldwin <jhb@freebsd.org> 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 <jhb@freebsd.org> 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
>> >> > <witness_warn+0x30e>
>> >> > 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
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTi=YjbBdZp9KuGUmMuYUmWyx_n%2BykikPSMMMo=j9>