Date: Sat, 21 May 2005 12:04:59 -0600 From: Scott Long <scottl@samsco.org> To: John-Mark Gurney <gurney_j@resnet.uoregon.edu> Cc: Hans Petter Selasky <hselasky@c2i.net> Subject: Re: problems with new the "contigmalloc" routine Message-ID: <428F784B.6010806@samsco.org> In-Reply-To: <20050521174408.GE959@funkthat.com> References: <200505201340.36176.hselasky@c2i.net> <200505202151.35831.hselasky@c2i.net> <20050520224928.GI2129@cirb503493.alcatel.com.au> <200505211546.56111.hselasky@c2i.net> <20050521174408.GE959@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
John-Mark Gurney wrote:
> 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...
>
I'd suggest just following a simplier and more deterministic path by
either pre-allocating your contiguous buffers in a safe context, or
allocating them on the fly before you depend on state being static.
Our concept of 'sleep' and 'block' are a bit muddled now that we have
liberal uses of sleep locks, and we as programmers need to cope and
adjust. It's always good form in embedded and kernel programming to
pre-allocate and closely manage resources when you can; this isn't
userland where resources are cheap.
Scott
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?428F784B.6010806>