Re: [PATCH] drm/ttm: Fix race condition in ttm_bo_delayed_delete

[Thomas Hellstrom <thomas-4+hqylr40dJg9hUCZPvPmw@public.gmane.org>]

Luca Barbieri wrote:
>> Would  nentry=list_first_entry(&entry->ddestroy, ....) work?
>>     
> Yes, it seems a bit less intuitive, but if that's the accepted
> practice, let's do that instead.
>
>   
>> Here nentry may have been removed from the list by another process, which
>> would trigger the unnecessary call, mentioned above.
>>     
> You are right.
>
> I attached a revised patch.
> It's only compile tested, but the changes are small and it should
> hopefully work.
>   
Yes, it looks correct. Although it seems a little unintuitive to enter 
the loop with the spinlock held, and exit it with the spinlock not held. 
I've attached yet another patch to have that fixed. Could you take a 
look at whether it seems OK with you and, in that case, repost it for Dave?

> The whole function seems more complicated than needed, but I can't
> find a way to do it with less code. If we could keep glob->lru_lock
> while calling ttm_bo_cleanup_refs things would be much simpler, but
> that would require intrusive changes and may not be possible.
>   

Yes, one of the rules of TTM is to avoid calling any kref_put() function 
that may free an object with a spinlock  or a mutex held,  since the 
free function must be able to take whatever mutex or spinlock it likes, 
otherwise we'd end up in a complete mess of recursive locks and lock 
dependency errors. Since therefore ttm_bo_cleanup_refs would need to 
temporarily release any lock held at call time, we wouldn't be better off.

/Thomas