Balancing ttm_mem_io_reserve() and ttm_mem_io_free()

[Thomas Hellström (Intel)]

Hi, Christian,

On 8/19/22 10:52, Christian König wrote:
> Hi Thomas,
>
> IIRC we intentionally dropped that approach of balancing 
> ttm_mem_io_reserve()/ttm_mem_io_free().
>
> Instead the results from ttm_mem_io_reserve() are cached and that 
> cached information is freed by ttm_mem_io_free(). In other words every 
> time we need to make sure we have the cache filled we call 
> ttm_mem_io_reserve() and everytime we are about to free the resource 
> or don't need the mapping any more we call ttm_mem_io_free().
>
> The callbacks to io_mem_reserve() and io_mem_free() are then balanced.

Hmm, yes, in the end at resource destroy, anything reserved would indeed 
have been freed, but consider the following:

ttm_bo_vm_fault();
ttm_bo_vmap();
ttm_bo_vunmap();
ttm_bo_unmap_virtual();

Here, wouldn't we release the io_reservation on ttm_bo_vunmap() when it 
should really have been released on ttm_bo_unmap_virtual()?


>
> Fixing missing _free() calls in the error path is probably a good 
> idea, but I wouldn't go beyond that.
>
> Why should any of that be racy? You need to hold the reservation lock 
> to call any of those functions.

It's when now a ttm_resource has been detached from a bo, and combined 
with an ongoing async memcpy we no longer have a bo reservation to 
protect with. Now the async memcpy currently only exists in i915 and we 
might at some point be able to get rid of it, but it illustrates the 
problem.

Thanks,

Thomas


>
> Regards,
> Christian.



>
> Am 19.08.22 um 10:13 schrieb Thomas Hellström:
>> Hi Christian,
>>
>> I'm looking for a way to take some sort of reference across possible 
>> VRAM accesses  over the PCI bar, be it for runtime PM, workarounds or 
>> whatever.
>>
>> The ttm_mem_io_reserve/free seems like a good candidate, but is 
>> completely unbalanced and looks racy. In particular error paths 
>> forget to call ttm_mem_io_free().
>>
>> Would you have any objections if I took a look at attempting to 
>> balance calls to those functions, or do you have any other 
>> suggestions for a better method?
>>
>> Thanks,
>>
>> Thomas
>>
>>
>>