Re: [Intel-gfx] [PATCH 00/18] dev->struct_mutex crusade
On Mon, Aug 10, 2015 at 01:35:58PM +0200, Thierry Reding wrote: > On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote: > > Hi all, > > > > I wanted to take another look at struct_mutex usage in modern (gem) drivers > > and > > noticed that for a fair lot we're very to be completely struct_mutex free. > > > > This pile here is the the simple part, which mostly just removes code and > > mutex_lock/unlock calls. All the patches here are independent and can be > > merged > > in any order whatsoever. My plan is to send out a pull request for all > > those not > > picked up by driver maintainers in 2-3 weeks or so, assuming no one > > complains. > > > > Of course review & comments still very much welcome. > > > > The more tricky 2nd part of this (and that one's not yet done) is to rework > > the > > gem mmap handler to use the same kref_get_unless_zero trick as ttm. With > > that > > there's no core requirement to hold struct_mutex over the final unref, which > > means we can make that one lockless. I plan to add a > > gem_object_free_unlocked > > for all the drivers which don't have any need for this lock. > > > > Also there's a few more drivers which can be made struct_mutex free easily, > > I'll > > propably stitch together poc patches for those. > > There's a concurrency bug in Tegra DRM currently because we don't lock > accesses to drm_mm (I guess this demonstrates how badly we need better > testing...) and it seems like this is typically protected by the very > same struct_mutex that you're on a crusade to remove. If your goal is > to get rid of it for good, should we simply add a separate lock just > for the drm_mm? We don't have another one that would fit. Yes, please just add your own driver-private lock. The next struct_mutex crusade series will actually do exactly that, at least for simple cases like armada. With that changes most drivers don't care about struct_mutex any more in their ->gem_free_object hook and I plan to add a new ->gem_free_object_unlocked variant for all the drivers which don't have any residual usage of struct_mutex. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/18] dev->struct_mutex crusade
On Mon, Aug 10, 2015 at 12:53:23PM +0100, Chris Wilson wrote: > On Mon, Aug 10, 2015 at 01:35:58PM +0200, Thierry Reding wrote: > > On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote: > > > Hi all, > > > > > > I wanted to take another look at struct_mutex usage in modern (gem) > > > drivers and > > > noticed that for a fair lot we're very to be completely struct_mutex free. > > > > > > This pile here is the the simple part, which mostly just removes code and > > > mutex_lock/unlock calls. All the patches here are independent and can be > > > merged > > > in any order whatsoever. My plan is to send out a pull request for all > > > those not > > > picked up by driver maintainers in 2-3 weeks or so, assuming no one > > > complains. > > > > > > Of course review & comments still very much welcome. > > > > > > The more tricky 2nd part of this (and that one's not yet done) is to > > > rework the > > > gem mmap handler to use the same kref_get_unless_zero trick as ttm. With > > > that > > > there's no core requirement to hold struct_mutex over the final unref, > > > which > > > means we can make that one lockless. I plan to add a > > > gem_object_free_unlocked > > > for all the drivers which don't have any need for this lock. > > > > > > Also there's a few more drivers which can be made struct_mutex free > > > easily, I'll > > > propably stitch together poc patches for those. > > > > There's a concurrency bug in Tegra DRM currently because we don't lock > > accesses to drm_mm (I guess this demonstrates how badly we need better > > testing...) and it seems like this is typically protected by the very > > same struct_mutex that you're on a crusade to remove. If your goal is > > to get rid of it for good, should we simply add a separate lock just > > for the drm_mm? We don't have another one that would fit. > > Actually that is one of the first targets for more fine-grained locking. > I would not add a new lock to drm_mm as at least for i915, we want to use > a similar per-vm lock (of which the drm_mm is just one part). Sorry if I was being unclear. I wasn't suggesting adding the lock to struct drm_mm, but rather add a driver-specific one specifically to serialize accesses to the drm_mm. I agree that it's better to do this in a driver-specific way because other structures may need to be protected by the same lock. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/18] dev->struct_mutex crusade
On Mon, Aug 10, 2015 at 01:35:58PM +0200, Thierry Reding wrote: > On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote: > > Hi all, > > > > I wanted to take another look at struct_mutex usage in modern (gem) drivers > > and > > noticed that for a fair lot we're very to be completely struct_mutex free. > > > > This pile here is the the simple part, which mostly just removes code and > > mutex_lock/unlock calls. All the patches here are independent and can be > > merged > > in any order whatsoever. My plan is to send out a pull request for all > > those not > > picked up by driver maintainers in 2-3 weeks or so, assuming no one > > complains. > > > > Of course review & comments still very much welcome. > > > > The more tricky 2nd part of this (and that one's not yet done) is to rework > > the > > gem mmap handler to use the same kref_get_unless_zero trick as ttm. With > > that > > there's no core requirement to hold struct_mutex over the final unref, which > > means we can make that one lockless. I plan to add a > > gem_object_free_unlocked > > for all the drivers which don't have any need for this lock. > > > > Also there's a few more drivers which can be made struct_mutex free easily, > > I'll > > propably stitch together poc patches for those. > > There's a concurrency bug in Tegra DRM currently because we don't lock > accesses to drm_mm (I guess this demonstrates how badly we need better > testing...) and it seems like this is typically protected by the very > same struct_mutex that you're on a crusade to remove. If your goal is > to get rid of it for good, should we simply add a separate lock just > for the drm_mm? We don't have another one that would fit. Actually that is one of the first targets for more fine-grained locking. I would not add a new lock to drm_mm as at least for i915, we want to use a similar per-vm lock (of which the drm_mm is just one part). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/18] dev->struct_mutex crusade
On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote: > Hi all, > > I wanted to take another look at struct_mutex usage in modern (gem) drivers > and > noticed that for a fair lot we're very to be completely struct_mutex free. > > This pile here is the the simple part, which mostly just removes code and > mutex_lock/unlock calls. All the patches here are independent and can be > merged > in any order whatsoever. My plan is to send out a pull request for all those > not > picked up by driver maintainers in 2-3 weeks or so, assuming no one complains. > > Of course review & comments still very much welcome. > > The more tricky 2nd part of this (and that one's not yet done) is to rework > the > gem mmap handler to use the same kref_get_unless_zero trick as ttm. With that > there's no core requirement to hold struct_mutex over the final unref, which > means we can make that one lockless. I plan to add a gem_object_free_unlocked > for all the drivers which don't have any need for this lock. > > Also there's a few more drivers which can be made struct_mutex free easily, > I'll > propably stitch together poc patches for those. There's a concurrency bug in Tegra DRM currently because we don't lock accesses to drm_mm (I guess this demonstrates how badly we need better testing...) and it seems like this is typically protected by the very same struct_mutex that you're on a crusade to remove. If your goal is to get rid of it for good, should we simply add a separate lock just for the drm_mm? We don't have another one that would fit. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
