Re: [PATCH 0/9] [RFC] fair-lru eviction

[Thomas Hellstrom <thomas@shipmail.org>]

On 05/19/2010 10:13 PM, Daniel Vetter wrote:
> On Wed, May 19, 2010 at 09:51:33PM +0200, Thomas Hellström wrote:
>    
>> Daniel,
>> TTM releases the spinlock protecting the drm_mm manager in between
>> evictions to be able to wait (without holding locks) for bo idle.
>> That means that the lru list may have changed between the first
>> eviction and the next. In practice, drivers either don't allow
>> simultaneous bo validation or should disallow it if thrashing is
>> likely to occur, so the likelyhood of the lru list changing in
>> between evictions should be small but it needs to be taken into
>> account.
>>      
> Well, in my opinion ttm locking optimize for the wrong case: Usually there
> should be a little bit of room free to fully pipeline everything. And if
> it there is no room free anymore, performance is gonna dip anyway, so we
> might as well block.

Yes, the driver is free to block in such cases if it wants to. It's a 
matter of taking a command submission rwsem in either read- or write 
mode. However, a validation sequence of one DRI client shouldn't 
unnecessarily block other renderers whose render targets  / sources are 
already in memory, but that's only a special case. TTM tries to make 
sure and encourage driver writers make sure no CPU locks are held while 
waiting for a GPU, which may turn out to be optimizing for the wrong 
case in a single-client-single-gpu environment, but turn out to be a 
good choice in other situations.

In any case, the code must take into account that the lru may be 
modified when the spinlock is released, which I believe you have 
addressed below.

>   I think the linux vm with the split between
> asynchronous background writeback and synchronous writeback in case of low
> amounts of free memory could be used as inspiration.
>
> Whatever, I think ttm could use this fair eviction stuff even with the
> current locking scheme: Do all the accounting under the spinlock, i.e.
> - scanning the lru
> - building up the eviction list
> - mark the memory blocks as free (with drm_mm_put_block)
> - reserve the complete free hole (needs a new function in drm_mm, but
>    range-restricted allocations are not yet implemented yet, anyway) with a
>    temporary object.
>
> Then do the effective eviction outside the spinlock. iirc ttm already uses
> such shadow (dunno what they're really called) objects for buffer moves.
>
>    
>> Nevertheless, the drm_mm.c cleanup is
>> Acked-by: Thomas Hellstrom<thellstrom@vmwgfx.com>
>>      
> Does that include the drm_mm_node->private pointer removal? Jerome naked
> that one (but I've objected). Just to clarify.
>
>    

IIRC, Jerome's range validation patches was using that member at some 
stage. I think if Jerome needs the pointer it should stay.

Thanks,
Thomas


>> I'll leave it to the Intel guys to comment on the fair eviction stuff.
>>
>> /Thomas
>>      
> Thanks, Daniel
>