Date: Sat, 7 Aug 2010 01:28:38 +0200 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: mdf@freebsd.org, freebsd-hackers@freebsd.org Subject: Re: sched_pin() versus PCPU_GET Message-ID: <AANLkTikOn%2BsvnsdqvR2Q=4eoo%2BZrT0-y7ah%2BncqawbOY@mail.gmail.com> In-Reply-To: <201008051312.25854.jhb@freebsd.org> References: <AANLkTikY20TxyeyqO5zP3zC-azb748kV-MdevPfm%2B8cq@mail.gmail.com> <201008041455.26066.jhb@freebsd.org> <AANLkTikvx9c=CjMcE7WsAZrxAxfqcDQEYOa0rWRBBXA5@mail.gmail.com> <201008051312.25854.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2010/8/5 John Baldwin <jhb@freebsd.org>:
> On Thursday, August 05, 2010 11:59:37 am mdf@freebsd.org wrote:
>> On Wed, Aug 4, 2010 at 11:55 AM, John Baldwin <jhb@freebsd.org> 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 <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> 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
>> >> >> >> > <witness_warn+0x30e>
>> >> >> >> > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikOn%2BsvnsdqvR2Q=4eoo%2BZrT0-y7ah%2BncqawbOY>