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 12:00:26 +0100 Message-ID: <53BFC3CA.3010703@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> 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 AE1BF6E161 for ; Fri, 11 Jul 2014 04:00:37 -0700 (PDT) In-Reply-To: <1405074481-4742-2-git-send-email-chris@chris-wilson.co.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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? Tvrtko