On 3/17/22 19:13, Rob Clark wrote: ...
/* prevent racing with job submission code paths */if (!dma_resv_trylock(obj->resv))goto shrinker_lock;jfwiw, the trylock here is in the msm code isn't so much for madvise (it is an error to submit jobs that reference DONTNEED objects), but instead for the case of evicting WILLNEED but inactive objects to swap. Ie. in the case that we need to move bo's back in to memory, we don't want to unpin/evict a buffer that is later on the list for the same job.. msm shrinker re-uses the same scan loop for both inactive_dontneed (purge) and inactive_willneed (evict)
I don't see connection between the objects on the shrinker's list and the job's BOs. Jobs indeed must not have any objects marked as DONTNEED, this case should never happen in practice, but we still need to protect from it.
Hmm, let me try to explain with a simple example.. hopefully this makes sense.
Say you have a job with two bo's, A and B.. bo A is not backed with memory (either hasn't been used before or was evicted. Allocating pages for A triggers shrinker. But B is still on the inactive_willneed list, however it is already locked (because we don't want to evict B to obtain backing pages for A).
I see now what you're talking about, thanks. My intention of locking the reservations is different since eviction isn't supported by this v2. But we probably will be able to re-use this try_lock for protecting from swapping out job's BOs as well.
I suppose using trylock is not technically wrong, and it would be a good idea if the shmem helpers supported eviction as well. But I think in the madvise/purge case if you lose the trylock then there is something else bad going on.
This trylock is intended for protecting job's submission path from racing with madvise ioctl invocation followed by immediate purging of BOs while job is in a process of submission, i.e. it protects from a use-after-free.
ahh, ok
If you'll lose this trylock, then shrinker can't use dma_resv_test_signaled() reliably anymore and shrinker may purge BO before job had a chance to add fence to the BO's reservation.
Anyways, from the PoV of minimizing lock contention when under memory pressure, this all looks good to me.
Thank you. I may try to add generic eviction support to the v3.
eviction is a trickier thing to get right, I wouldn't blame you for splitting that out into it's own patchset ;-)
You probably also would want to make it a thing that is opt-in for drivers using the shmem helpers
I had the same thoughts, will see.