On Fri, Jan 31, 2025 at 11:14:06AM +1100, Alistair Popple wrote: > On Thu, Jan 30, 2025 at 04:29:33PM +0100, David Hildenbrand wrote: > > On 30.01.25 14:31, Simona Vetter wrote: > > > On Thu, Jan 30, 2025 at 10:37:06AM +0100, David Hildenbrand wrote: > > > > On 30.01.25 01:27, Alistair Popple wrote: > > > > > On Wed, Jan 29, 2025 at 12:58:02PM +0100, David Hildenbrand wrote: > > > > > > Let's document how this function is to be used, and why the requirement > > > > > > for the folio lock might maybe be dropped in the future. > > > > > > > > > > Sorry, only just catching up on your other thread. The folio lock was to ensure > > > > > the GPU got a chance to make forward progress by mapping the page. Without it > > > > > the CPU could immediately invalidate the entry before the GPU had a chance to > > > > > retry the fault. > > > > > > Obviously performance wise having such thrashing is terrible, so should > > > > > really be avoided by userspace, but the lock at least allowed such programs > > > > > to complete. > > > > > > > > Thanks for the clarification. So it's relevant that the MMU notifier in > > > > remove_device_exclusive_entry() is sent after taking the folio lock. > > > > > > > > However, as soon as we drop the folio lock, remove_device_exclusive_entry() > > > > will become active, lock the folio and trigger the MMU notifier. > > > > > > > > So the time it is actually mapped into the device is rather > > > > I meant to say "rather short." :) > > > > > > > > Looks like you cut off a bit here (or mail transport did that somewhere), > > > but see my other reply I don't think this is a legit use-case. So we don't > > > have to worry. > > > > In that case, we would need the folio lock in the future. > > > > > Well beyond documenting that if userspace concurrently thrashes > > > the same page with both device atomics and cpu access it will stall real > > > bad. > > > > I'm curious, is locking between device-cpu or device-device something that > > can happen frequently? In that case, you would get that trashing naturally? > > It results in terrible performance so in practice it isn't something that I've > seen except when stress testing the driver. Those stress tests were useful for > exposing a range of kernel/driver bugs/issues though, and despite the short time > it is mapped the lock was sufficient to allow atomic thrashing tests to complete > vs. having the device fault endlessly. Yeah the practical use-case of device atomics is that they alternate, as the processing shifts between the cpu and the gpu. Classic one is when you generate variable amounts of data per input block that you fill into a big preallocated array, and the atomic counter is your dumb-as-rock malloc for that. The atomic moves as the generating/consuming of that data moves around the system (and needs a fault each time it moves), but you really never want to have concurrent access going on. Any thrashing concurrent access is just broken userspace (or a driver stress test). > So unless it's making things more difficult I'd rather keep the lock. But why do these stress-tests need to complete? We have a lot of these in our gpu test suite, and we just nuke them after a short timeout if nothing blows up. Concurrently hammering the same page from both gpu and cpu really isn't something that should have any forward progress gurantees imo. And I feel like too much locking just risks us hiding some much more nasty (design) bugs underneath - I've had that experience reviewing both amdkfd and the in-work xe code. So my preference for gpu interacting with core mm is that we have the least amount of locking to make sure we really don't have a breaking design impendence mismatch going on. Cheers, Sima -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
