On Thu, 17 Mar 2011 13:54:45 -0300, Herton Ronaldo Krzesinski herton.krzesinski@canonical.com wrote:
On Thu, Mar 17, 2011 at 01:46:34PM +0000, Chris Wilson wrote:
This is the single chunk required. I had thought that the actual insertion/deletion was serialised under the struct mutex and the intention of the spinlock was to protect the unlocked list traversal during throttling. However, I missed that i915_gem_release() is also called without struct mutex and so we do need the double check for i915_gem_request_remove_from_client().
Ok. I just still have one doubt though, if in i915_add_request file/file_priv is NULL, wouldn't be possible to have an oops also in i915_gem_release without the check? As in this case, request->client_list wouldn't have mm.request_list added to it, and if an error occurs and i915_reset is called, which ends up calling i915_gem_release, we would try to do a list_del on request->client_list without items.
If the file_priv is NULL, then the request is not added to the client mm.request_list and so it is not seen during i915_gem_release.
The list is file_priv->mm.request_list, the nodes within that are request->client_list.
If the check really isn't needed in i915_gem_release, then please consider this patch:
Done, thanks, -Chris