From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tvrtko Ursulin Subject: Re: [PATCH 2/2] drm/i915: Initialise userptr mmu_notifier serial to 1 Date: Fri, 11 Jul 2014 15:06:55 +0100 Message-ID: <53BFEF7F.9000607@linux.intel.com> References: <1405074481-4742-1-git-send-email-chris@chris-wilson.co.uk> <1405074481-4742-2-git-send-email-chris@chris-wilson.co.uk> <53BFC3CA.3010703@linux.intel.com> <20140711110602.GB12165@nuc-i3427.alporthouse.com> <20140711140217.GZ17271@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 480A36E836 for ; Fri, 11 Jul 2014 07:06:59 -0700 (PDT) In-Reply-To: <20140711140217.GZ17271@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter , Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 07/11/2014 03:02 PM, Daniel Vetter wrote: > On Fri, Jul 11, 2014 at 12:06:02PM +0100, Chris Wilson wrote: >> On Fri, Jul 11, 2014 at 12:00:26PM +0100, Tvrtko Ursulin wrote: >>> >>> On 07/11/2014 11:28 AM, Chris Wilson wrote: >>>> During the range invalidate, we walk the list of buffers associated with >>>> the mmu_notifer and find the ones that overlap the range. An >>>> optimisation is made to speed up the iteration by assuming the previous >>>> iter is still valid whilst the tree is unmodified. This exposes a bug >>>> when a range invalidate is triggered after we have just created the >>>> mmu_notifier, but before attaching any buffers. In that case, we presume >>>> we have an unmodified list and start walking from the last iter which is >>>> NULL. Oops. >>>> >>>> The easiest fix is then to initialise the serial of the tree to 1. >>>> >>>> Signed-off-by: Chris Wilson >>>> Cc: Tvrtko Ursulin >>>> --- >>>> drivers/gpu/drm/i915/i915_gem_userptr.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c >>>> index 7c38f50014db..8e9e91029aed 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c >>>> @@ -197,7 +197,7 @@ i915_mmu_notifier_get(struct drm_device *dev, struct mm_struct *mm) >>>> mmu->mm = mm; >>>> mmu->objects = RB_ROOT; >>>> mmu->count = 0; >>>> - mmu->serial = 0; >>>> + mmu->serial = 1; >>>> INIT_LIST_HEAD(&mmu->linear); >>>> mmu->is_linear = false; >>>> >>> >>> Looks good to me and I think safe to merge in any case, so: >>> >>> Reviewed-by: Tvrtko Ursulin >>> >>> But it will be interesting to know what code managed to trigger this >>> race, because as we discussed on IRC it would indicate some pretty >>> wild userspace behaviour. Or lack of imagination on our part? >> >> A threaded client. One thread using userptr, the other doing munmap or >> free. Given enough embarrassment, it will happen every time. > > Can I have an igt please to confirm this and ensure it stays fixed? Sure, give me a day or two. Tvrtko