On Sun, Jun 30, 2013 at 02:04:56PM +0100, Russell King - ARM Linux wrote: > On Sun, Jun 30, 2013 at 02:37:41PM +0200, Daniel Vetter wrote: > > On Sat, Jun 29, 2013 at 11:53:22PM +0100, Russell King wrote: > > > + mutex_lock(&dev->struct_mutex); > > > + free = drm_mm_search_free(&priv->linear, size, align, 0); > > > + if (free) > > > + obj->linear = drm_mm_get_block(free, size, align); > > > + mutex_unlock(&dev->struct_mutex); > > > > The two-stage search_free+get_block drm_mm interface is something I'd like > > to remove since it' complicates the interface for no gain. And the > > preallocation trick for atomic contexts is pretty racy as-is. Can you > > please convert this over to the insert_node interfaces which take a > > preallocate drm_mm_node? > > Yes and no. This is only racy if it is callable from atomic contexts, > which the above isn't, because it takes a mutex, and the above is the > only place which allocations against that pool are done. Mutexes can't > be taken in atomic contexts. Plus it's using the non-atomic drm_mm_* > interfaces. > > However, the insert_node interfaces appear not to solve any kind of race. > All they do is allow the kmalloc to be moved out of this region into > the user of this code sequence, while offering no additional locking or > safety. So the mutex lock around a call to drm_mm_insert_node*() is > still required. > > As the kmalloc() happens beneath the mutex lock, another thread can't race > with an allocation in the above code anyway. The *only* reason I'll change > this is that it is nicer to the system not to hold locks while allocating > memory. Err. There's bugs here in the other drivers such as i915 which follow your suggestion. The problem is this: Every time they allocate using drm_mm_insert_node(), they kmalloc a new struct drm_mm_node for this function to fill in and attach. When they've done with the allocation, they call drm_mm_put_block(). This places the node onto the mm's unused_nodes list provided we don't already have MM_UNUSED_TARGET nodes on that list. So, we end up filling up the unused nodes list to MM_UNUSED_TARGET, at which point we then start freeing any further nodes - and with no way to pull these off that list, they all just sit there not being used. The only function which takes nodes off that list is drm_mm_kmalloc(), which is declared statically, and so isn't available to drivers. Are drivers which use the drm_mm_insert_node() function expected to also use drm_mm_remove_node(), kfreeing the node after that call, rather than using drm_mm_put_block() ? Either way, I think someone needs to fix the other drivers and Cc those patches to Stable... _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
