Date: Thu, 4 Aug 2005 09:53:58 -0400 From: John Baldwin <jhb@FreeBSD.org> To: hselasky@c2i.net Cc: freebsd-hackers@freebsd.org Subject: Re: How to do proper locking Message-ID: <200508040953.59316.jhb@FreeBSD.org> In-Reply-To: <200508041340.58851.hselasky@c2i.net> References: <200508030023.04748.hselasky@c2i.net> <200508031321.32276.jhb@FreeBSD.org> <200508041340.58851.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thursday 04 August 2005 07:40 am, Hans Petter Selasky wrote:
> On Wednesday 03 August 2005 19:21, John Baldwin wrote:
> > On Tuesday 02 August 2005 06:23 pm, Hans Petter Selasky wrote:
> > > Hi,
> > >
> > > I am looking for a safe way to access structures that can be freed. The
> > > solution I am looking for must not:
> > >
> > > - hinder scaleability
> > > - lead to use of a single lock
> > > - lead to lock order reversal
>
> There is one more thing. When the structure is freed it should not block
> waiting for any reference counts.
>
> > These aren't a very clear set of requirements.
>
> If you think about the situation of accessing a structure in parallell with
> freeing it, and want to do it 100 percent safe, you will see that you have
> to do some kind of test before accessing this structure to see if it is
> still present. So how should this test look like if you want to keep the
> points I've listed above in mind?
>
> > Note that you can't hold a
> > lock across malloc(), so your allocation code isn't safe.
>
> I was thinking that the M_NOWAIT flag was passed malloc(). Else in this
> case, I think it is safe to unlock/lock "lock_A" when malloc is called.
>
> > You can try taking at look at some existing refcounted structures such
> > as ucreds, p_args, etc. or looking at structures held in a global list
> > like proc.
>
> It looks to me like some parent lock is held, or that these structures are
> only accessed from a single thread, to prevent synchronization problems.
You hold a parent lock (which is what your lock_A is if you think about it)
while you bump the reference count, yes.
> This is a copy and paste from the kernel sources:
>
> struct ucred *
> crhold(struct ucred *cr)
> {
> The problem is, what happens if the kernel switches thread right here, and
> then the other thread calls "crfree()" on this structure, that will
> "free()" memory pointed to by "cr". Then the first line of code below will
> access freed memory, and then we are going for a segment fault or worse.
>
> mtx_lock(cr->cr_mtxp);
> cr->cr_ref++;
> mtx_unlock(cr->cr_mtxp);
> return (cr);
> }
crhold() is always called when the caller knows that it has a reference to cr
that can't go away. Thus, in the case of p_ucred, one holds the associated
PROC_LOCK() while doing crhold(). After the crhold() you can drop the proc
lock, so that lock is only held for a very short period of time. You then
have your own reference to the cred structure that is valid until you call
crfree(). In the case of curthread->td_ucred, the reference is valid until
curthread releases it on a cred update at the start of a syscall, so your
reference is valid without needing locks for the life of a syscall. The key
here is that you solve this problem at a higher level, not within the
structure itself.
> > What actual problem are you trying to solve?
>
> Generic callbacks:
>
> - setting up timers. Make sure that callback is never called after that
> timer has been stopped.
See callout_stop() and callout_drain(). callout_drain() won't return until
the callout has both been stopped and has finished executing if it was
already in progress.
> - setting up devices. Make sure that read/write/ioctl/poll callbacks are
> never called after that the device has been freed.
This is managed with refcounts on the dev_t object by devfs. Once you call
destroy_dev() devfs manages the rest.
> - setting up interrupts. Make sure that interrupt routine is never called
> after that it is been teared down.
bus_teardown_intr() already does this.
> And more. There are lots of situations in the kernel that run into the same
> scheme, without a proper solution.
Are there any other places you can think of that don't handle this already?
The approach FreeBSD's kernel has taken is to solve this problem by
duplicating a lot of checks all over the place for each data structure, but
to solve it in the primitives themselves instead (hence callout_drain(),
bus_teardown_intr(), destroy_dev(), etc.)
--
John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200508040953.59316.jhb>