Re: buggy/weird behavior in ttm

[Thomas Hellstrom <thellstrom@vmware.com>]

On 10/11/2012 08:42 PM, Maarten Lankhorst wrote:

>
>> Anyway, if you plan to remove the fence lock and protect it with reserve, you must
>> make sure that a waiting reserve is never done in a destruction path. I think this
>> mostly concerns the nvidia driver.
> Well I don't think any lock should ever be held during destruction time,

What I mean is, that *something* needs to protect the fence pointer. 
Currently it's the
fence lock, and I was assuming you'd protect it with reserve. And 
neither TTM nor
Nvidia should, when a resource is about to be freed, be forced to 
*block* waiting for
reserve just to access the fence pointer. When and if you have a 
solution that fulfills
those requirements, I'm ready to review it.

>
>>> - no_wait_reserve is ignored if no_wait_gpu is false
>>>     ttm_bo_reserve_locked can only return true if no_wait_reserve is true, but
>>>     subsequently it will do a wait_unreserved if no_wait_gpu is false.
>>> I'm planning on removing this argument and act like it is always true, since
>>> nothing on the lru list should fail to reserve currently.
>> Yes, since all buffers that are reserved are removed from the LRU list, there
>> should never be a waiting reserve on them, so no_wait_reserve can be removed
>> from ttm_mem_evict_first, ttm_bo_evict and possibly other functions in the call chain.
> I suppose there will stay a small race though,

Hmm, where?

>>> - effectively unlimited callchain between some functions that all go through
>>>     ttm_mem_evict_first:
>>>
>>>                                       /------------------------\
>>> ttm_mem_evict_first - ttm_bo_evict -                          -ttm_bo_mem_space  - ttm_bo_mem_force_space - ttm_mem_evict_first
>>>                                       \ ttm_bo_handle_move_mem /
>>> I'm not surprised that there was a deadlock before, it seems to me it would
>>> be pretty suicidal to ever do a blocking reserve on any of those lists,
>>> lockdep would be all over you for this.
>> Well, at first this may look worse than it actually is. The driver's eviction memory order determines the recursion depth
>> and typically it's 0 or 1, since subsequent ttm_mem_evict_first should never touch the same LRU lists as the first one.
>> What would typically happen is that a BO is evicted from VRAM to TT, and if there is no space in TT, another BO is evicted
>> to system memory, and the chain is terminated. However a driver could set up any eviction order but that would be
>> a BUG.
>>
>> But in essence, as you say, even with a small recursion depth, a waiting reserve could cause a deadlock.
>> But there should be no waiting reserves in the eviction path currently.
> Partially true, ttm_bo_cleanup_refs is currently capable of blocking reserve.
> Fixing this might mean that ttm_mem_evict_first may need to become more aggressive,
> since it seems all the callers of this function assume that ttm_mem_evict_first can only fail
> if there is really nothing more to free and blocking nested would really upset lockdep
> and leave you open to the same deadlocks.
I can't see how the waiting reserve in ttm_bo_cleanup_refs would cause a 
deadlock,
because the buffer about to be reserved is always *last* in a 
reservation sequence, and the
reservation is always released (or the buffer destroyed) before trying 
to reserve another buffer.
Technically the buffer is not looked up from a LRU list but from the 
delayed delete list.
Could you describe such a deadlock case?
(There is a bug in there, though that the reservation is not released if 
the buffer is no longer
on the reservation list here):

if (unlikely(ret != 0) || list_empty(&bo->ddestroy)) {
      spin_unlock(&glob->lru_lock);
      return ret;
}

>
> An unexpected bonus from adding this skipping would be that the atomic lru_list
> removal on unreserve guarantee becomes a lot easier to drop while keeping behavior
> exactly the same. Only the swapout code would need to be adjusted for to try the whole
> list. Maybe ttm_bo_delayed_delete with remove_all = false too would change behavior
> slightly too, but this seems to be less likely, as it could only ever happen on delayed
> destruction of imported dma-buf's.

May I kindly remind you that the atomic lru_list removal stays in TTM
until we've had a high level design discussion weighing it against an 
extended
reservation object API.


>
> Not true, ttm_bo_move_memcpy has no interruptible parameter, so it's not compatible with driver.move.
> The drivers that do use this function have set up their own alias that calls that function instead,
> so I was thinking of dropping those parameters from memcpy since they're unused and the drivers
> that could point their move member to it don't do it, so no point in having compatibility...

OK, then I'm OK with parameter changes in those functions.

>
> ~Maarten
>