Subject: Re: lock bug in getnewvnode, or uvm_km_kmemalloc/uvm_map ?
To: Bill Studenmund <wrstuden@netbsd.org>
From: Andrew Brown <atatat@atatdot.net>
List: tech-kern
Date: 11/21/2002 10:55:10
>> ...uvm_map() calls amap_extend() to extend an already existing amap.
>> that requires that the process is running (ie, not starting up) and
>> has caused the amap to be allocated.  this sounds like something that
>> occurs very much after raid autoconfig time.  additionally, since
>> we're talking about 1.6, this means the process is extending the amap
>> forwards to cover a new allocation (since the backwards extension code
>> is new), so the process is already well under way (ie, past
>> sys_execve() and past the initial runtime of the dynamic linker).  for
>> 1.6, that sounds well into multi-user.
>
>I'm confused. How does that relate to not wanting to sleep with raidframe
>locks held?

this all started like this:

>trying to backport the raidrame lock fixes to 1.6, I ran into a panic
>at raid autoconfig time, because pool_get(PR_WAITOK) is called with a
>simple_lock held.
>
>The call trace is:
>getnewvnode() (the simple_lock is grabbed in getnewvnode)
>pool_get(PR_NOWAIT)
>pool_allocator_alloc()
>pool_page_alloc_nointr()
>uvm_km_alloc_poolpage1()
>uvm_km_kmemalloc()
>uvm_map()
>pool_get(PR_WAITOK)
>
>The bug is, although pool_get() was initially called PR_NOWAIT, the pool
>memory allocator will call pool_get(PR_WAITOK). This is triggered by
>raidframe and getnewvnode(), but I think it's not specific to it, and
>I think it's still present in current.
>
>uvm_km_kmemalloc() is properly called with UVM_KMF_NOWAIT.
>But uvm_map() is called with flags stripped to UVM_KMF_TRYLOCK, so
>UVM_KMF_NOWAIT is lost here.
>Second problem, uvm_mapent_alloc() called from uvm_map() will call
>pool_get(PR_WAITOK) unconditionally. It seems to me that uvm_map() was not   
>designed to be called from a context where we can sleep, but I may be
>wrong.

i can't say for sure whether uvm_map() was designed to be called from
a context where we can sleep, but these two things struck me when i
saw manuel's patch.

(1) amap_extend() is only called in process context, meaning that we
*can* sleep.

(2) raid autoconfig takes place (correct me if i'm wrong here) at the
end of the boot process, but before init is started, so it's *not* a
process context.

i was trying to say that the two things are mutually exclusive, and
that manuel should not expend the effort to "fix" amap_extend() for a
context in which it will never be used.

-- 
|-----< "CODE WARRIOR" >-----|
codewarrior@daemon.org             * "ah!  i see you have the internet
twofsonet@graffiti.com (Andrew Brown)                that goes *ping*!"
werdna@squooshy.com       * "information is power -- share the wealth."