problems with new the "contigmalloc" routine

[Scott Long]

Hans Petter Selasky wrote:

> On Saturday 21 May 2005 20:04, Scott Long wrote:
> 
>>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);
> 
> 
> When I worked with USB I ran into some synchronization problems with callbacks 
> that leaded me to writing to the stack of another thread, which I hope is OK. 

No, it's not ok.  Kernel stacks can be swapped out while sleeping. 
Writing to a swapped out stack will cause a VM panic.

> What do you think about the following:
> 
[...]
> 
> I hope this wasn't too much for you to read :-)
> 
> Any comments ?
> 
> 
> 
>>>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...
> 
> 
> right
> 
> 
>>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.
> 
> 
> Pre-allocating memory is good, but not always the easiest to do ...
> 
> --HPS

I'm not too familiar with the exact problem you're trying to solve in 
USB.  I guess you need to be able to allocate a contiguous chunk of
memory in order to do a transaction, but are afraid that state will
change while the allocation is in progress?  What is the maximum size
of memory that the hardware can handle for this transaction?  How many
transactions can be handled concurrently by the hardware?  What state
are you trying to protect?  Is it possible to do the allocation before
the state needs to be protected?

Scott