Date: Thu, 29 Jul 2010 16:57:25 -0700 From: mdf@FreeBSD.org To: freebsd-hackers@freebsd.org Subject: Re: sched_pin() versus PCPU_GET Message-ID: <AANLkTimGjNATWmuGqTDMFQ0r3gHnsv0Bc69pBb6QYO9L@mail.gmail.com> In-Reply-To: <AANLkTikY20TxyeyqO5zP3zC-azb748kV-MdevPfm%2B8cq@mail.gmail.com> References: <AANLkTikY20TxyeyqO5zP3zC-azb748kV-MdevPfm%2B8cq@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Jul 29, 2010 at 4:39 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 =A0 =A0thread_lock(td);
> =A0 =A0 =A0 =A0td->td_flags &=3D ~TDF_SELECT;
> =A0 =A0 =A0 =A0thread_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 =A0 =A0 65 48 8b 1c 25 00 00 =A0 =A0mov =A0 =A0%gs:=
0x0,%rbx
> ffffffff802db217: =A0 =A0 =A0 00 00
> ffffffff802db219: =A0 =A0 =A0 ff 83 04 01 00 00 =A0 =A0 =A0 incl =A0 0x10=
4(%rbx)
> =A0 =A0 =A0 =A0 * Pin the thread in order to avoid problems with thread m=
igration.
> =A0 =A0 =A0 =A0 * Once that all verifies are passed about spinlocks owner=
ship,
> =A0 =A0 =A0 =A0 * the thread is in a safe path and it can be unpinned.
> =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0sched_pin();
> =A0 =A0 =A0 =A0lock_list =3D PCPU_GET(spinlocks);
> ffffffff802db21f: =A0 =A0 =A0 65 48 8b 04 25 48 00 =A0 =A0mov =A0 =A0%gs:=
0x48,%rax
> ffffffff802db226: =A0 =A0 =A0 00 00
> =A0 =A0 =A0 =A0if (lock_list !=3D NULL && lock_list->ll_count !=3D 0) {
> ffffffff802db228: =A0 =A0 =A0 48 85 c0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0tes=
t =A0 %rax,%rax
> =A0 =A0 =A0 =A0 * Pin the thread in order to avoid problems with thread m=
igration.
> =A0 =A0 =A0 =A0 * Once that all verifies are passed about spinlocks owner=
ship,
> =A0 =A0 =A0 =A0 * the thread is in a safe path and it can be unpinned.
> =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0sched_pin();
> =A0 =A0 =A0 =A0lock_list =3D PCPU_GET(spinlocks);
> ffffffff802db22b: =A0 =A0 =A0 48 89 85 f0 fe ff ff =A0 =A0mov =A0 =A0%rax=
,-0x110(%rbp)
> ffffffff802db232: =A0 =A0 =A0 48 89 85 f8 fe ff ff =A0 =A0mov =A0 =A0%rax=
,-0x108(%rbp)
> =A0 =A0 =A0 =A0if (lock_list !=3D NULL && lock_list->ll_count !=3D 0) {
> ffffffff802db239: =A0 =A0 =A0 0f 84 ff 00 00 00 =A0 =A0 =A0 je =A0 =A0 ff=
ffffff802db33e
> <witness_warn+0x30e>
> ffffffff802db23f: =A0 =A0 =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 migrated
> 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 IIRC.
>
> So... do we need some kind of barrier in the code for sched_pin() for
> it to really do what it claims? =A0Could the hardware have re-ordered
> the "mov =A0 =A0%gs:0x48,%rax" PCPU_GET to before the sched_pin()
> increment?
So after some research, the answer I'm getting is "maybe". What I'm
concerned about is whether the h/w reordered the read of PCPU_GET in
front of the previous store to increment td_pinned. While not an
ultimate authority,
http://en.wikipedia.org/wiki/Memory_ordering#In_SMP_microprocessor_systems
implies that stores can be reordered after loads for both Intel and
amd64 chips, which would I believe account for the behavior seen here.
Thanks,
matthew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTimGjNATWmuGqTDMFQ0r3gHnsv0Bc69pBb6QYO9L>