Date: Sat, 21 May 2005 10:44:08 -0700 From: John-Mark Gurney <gurney_j@resnet.uoregon.edu> To: Hans Petter Selasky <hselasky@c2i.net> Cc: hackers@freebsd.org Subject: Re: problems with new the "contigmalloc" routine Message-ID: <20050521174408.GE959@funkthat.com> In-Reply-To: <200505211546.56111.hselasky@c2i.net> References: <200505201340.36176.hselasky@c2i.net> <200505202151.35831.hselasky@c2i.net> <20050520224928.GI2129@cirb503493.alcatel.com.au> <200505211546.56111.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
Hans Petter Selasky wrote this message on Sat, May 21, 2005 at 15:46 +0200:
> On Saturday 21 May 2005 00:49, Peter Jeremy wrote:
> > On Fri, 2005-May-20 21:51:34 +0200, Hans Petter Selasky wrote:
> > >Non-blocking mode has got to be supported. Else you get a nightmare
> > > rewriting the code to support blocking mode.
> >
> > Your code either has to block somewhere or fail. contigmalloc() only
> > blocks if it couldn't satisfy the request immediately. If it returns
> > to your code, you still have the problem of needing the memory and
> > not being able to allocate it without blocking.
>
> That is not the problem. The problem is that it sleeps, which will exit the
> Giant lock, which means another thread can change the state of what I am
> about to setup meanwhile:
>
> one_thread:
>
> mtx_lock(&Giant);
>
> if(sc->gone == 0)
> {
> sc->data = contigmalloc();
> }
>
> mtx_unlock(&Giant);
>
> another_thread:
>
> mtx_lock(&Giant);
>
> if(sc->data)
> {
> contigfree();
> sc->data = NULL;
> }
>
> sc->gone = 1;
>
> mtx_unlock(&Giant);
>
>
> The problem is that the undefined state: "sc->data != NULL" and
> "sc->gone == 1" can be reached.
How about rewriting the code to be:
one_thread:
tmpdata = contigmalloc();
mtx_lock(&Giant);
if(sc->gone == 0) {
sc->data = tmpdata;
} else {
contigfree(tmpdata);
}
mtx_unlock(&Giant);
another_thread:
mtx_lock(&Giant);
if(sc->data) {
tmpdata = sc->data;
sc->data = NULL;
}
sc->gone = 1;
mtx_unlock(&Giant);
contigfree(tmpdata);
That should do it.. Though you do need to have your own ref count on sc
to prevent the entire sc from going away before the first thread has
locked... Anyways, you should be using your own lock that's in sc for
this instead of using Giant...
Remeber that you can usually do some of the work before obtaining the
lock, and then swapping a few variables around... In fact, it's usually
better to do this since you don't have to do as much work with the lock
locked...
I don't think -current should allow new code to use Giant...
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050521174408.GE959>