Date: Tue, 26 Jan 2010 17:17:00 -0500 From: John Baldwin <jhb@freebsd.org> To: Attilio Rao <attilio@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, xcllnt@mac.com, marcel@freebsd.org, svn-src-head@freebsd.org, "M. Warner Losh" <imp@bsdimp.com> Subject: Re: svn commit: r202889 - head/sys/kern Message-ID: <201001261717.00070.jhb@freebsd.org> In-Reply-To: <3bbf2fe11001261320k654b2b8ck3d03c4d94ae8d9c1@mail.gmail.com> References: <3bbf2fe11001260058i65604619l664bd0e49c1dbbd@mail.gmail.com> <201001261551.52206.jhb@freebsd.org> <3bbf2fe11001261320k654b2b8ck3d03c4d94ae8d9c1@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tuesday 26 January 2010 4:20:46 pm Attilio Rao wrote:
> 2010/1/26 John Baldwin <jhb@freebsd.org>:
> > On Tuesday 26 January 2010 3:09:32 pm M. Warner Losh wrote:
> >> In message: <C6A8F7A7-F0A9-4F63-B61E-DDC5332DC495@mac.com>
> >> Marcel Moolenaar <xcllnt@mac.com> writes:
> >> : Maybe what is in order right now is a description (using pseudo
> >> : code if you like) of what exactly needs to happen with the 3rd
> >> : argument, when and how (i.e. what must be atomic and what does
> >> : not have to be atomic).
> >>
> >> I believe the proper pseudo code should be:
> >>
> >> cpu_switch(struct thread *old, struct thread *new, struct mutext *mtx)
> >> {
> >> /* Save the registers to the pcb */
> >> old->td_lock = mtx;
> >> #if defined(SMP) && defined(SCHED_ULE)
> >> /* s/long/int/ if sizeof(long) != sizeof(void *) */
> >> /* as we have no 'void *' version of the atomics */
> >> while (atomic_load_acq_long(&new->td_lock) == (long)&blocked_lock)
> >> continue;
> >> #endif
> >> /* Switch to new context */
> >> }
> >
> > FYI, that is what the '_ptr' variants of atomic ops are for. I do think that
> > the 'acq' membar on the load is needed to ensure the CPU doesn't try to
> > speculatively read any of the 'new' thread's PCB until it sees the update to
> > td_lock.
>
> I think it is more important to store the old lock via a rel barrier instead:
>
> - old->td_lock = mtx;
> + atomic_store_rel_ptr(&old->td_lock, mtx);
Both are required to ensure the new CPU does not load stale values from the pcb.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201001261717.00070.jhb>