[FIXES 1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[FIXES 1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[Chris Wilson]

set_page_dirty says:

	For pages with a mapping this should be done under the page lock
	for the benefit of asynchronous memory errors who prefer a
	consistent dirty state. This rule can be broken in some special
	cases, but should be better not to.

Under those rules, it is only safe for us to use the plain set_page_dirty
calls for shmemfs/anonymous memory. Userptr may be used with real
mappings and so needs to use the locked version (set_page_dirty_lock).

However, following a try_to_unmap() we may want to remove the userptr and
so call put_pages(). However, try_to_unmap() acquires the page lock and
so we must avoid recursively locking the pages ourselves -- which means
that we cannot safely acquire the lock around set_page_dirty(). Since we
can't be sure of the lock, we have to risk skip dirtying the page, or
else risk calling set_page_dirty() without a lock and so risk fs
corruption.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203317
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112012
Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video m
References: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
References: 505a8ec7e11a ("Revert "drm/i915/userptr: Acquire the page lock around set_page_dirty()"")
References: 6dcc693bc57f ("ext4: warn when page is dirtied without buffers")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 22 ++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index ee65c6acf0e2..dd104b0e2071 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -646,8 +646,28 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 		obj->mm.dirty = false;
 
 	for_each_sgt_page(page, sgt_iter, pages) {
-		if (obj->mm.dirty)
+		if (obj->mm.dirty && trylock_page(page)) {
+			/*
+			 * As this may not be anonymous memory (e.g. shmem)
+			 * but exist on a real mapping, we have to lock
+			 * the page in order to dirty it -- holding
+			 * the page reference is not sufficient to
+			 * prevent the inode from being truncated.
+			 * Play safe and take the lock.
+			 *
+			 * However...!
+			 *
+			 * The mmu-notifier can be invalidated for a
+			 * migrate_page, that is alreadying holding the lock
+			 * on the page. Such a try_to_unmap() will result
+			 * in us calling put_pages() and so recursively try
+			 * to lock the page. We avoid that deadlock with
+			 * a trylock_page() and in exchange we risk missing
+			 * some page dirtying.
+			 */
 			set_page_dirty(page);
+			unlock_page(page);
+		}
 
 		mark_page_accessed(page);
 		put_page(page);
-- 
2.24.0

[Intel-gfx] [FIXES 1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[Chris Wilson]

set_page_dirty says:

	For pages with a mapping this should be done under the page lock
	for the benefit of asynchronous memory errors who prefer a
	consistent dirty state. This rule can be broken in some special
	cases, but should be better not to.

Under those rules, it is only safe for us to use the plain set_page_dirty
calls for shmemfs/anonymous memory. Userptr may be used with real
mappings and so needs to use the locked version (set_page_dirty_lock).

However, following a try_to_unmap() we may want to remove the userptr and
so call put_pages(). However, try_to_unmap() acquires the page lock and
so we must avoid recursively locking the pages ourselves -- which means
that we cannot safely acquire the lock around set_page_dirty(). Since we
can't be sure of the lock, we have to risk skip dirtying the page, or
else risk calling set_page_dirty() without a lock and so risk fs
corruption.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203317
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112012
Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video m
References: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
References: 505a8ec7e11a ("Revert "drm/i915/userptr: Acquire the page lock around set_page_dirty()"")
References: 6dcc693bc57f ("ext4: warn when page is dirtied without buffers")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 22 ++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index ee65c6acf0e2..dd104b0e2071 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -646,8 +646,28 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
 		obj->mm.dirty = false;
 
 	for_each_sgt_page(page, sgt_iter, pages) {
-		if (obj->mm.dirty)
+		if (obj->mm.dirty && trylock_page(page)) {
+			/*
+			 * As this may not be anonymous memory (e.g. shmem)
+			 * but exist on a real mapping, we have to lock
+			 * the page in order to dirty it -- holding
+			 * the page reference is not sufficient to
+			 * prevent the inode from being truncated.
+			 * Play safe and take the lock.
+			 *
+			 * However...!
+			 *
+			 * The mmu-notifier can be invalidated for a
+			 * migrate_page, that is alreadying holding the lock
+			 * on the page. Such a try_to_unmap() will result
+			 * in us calling put_pages() and so recursively try
+			 * to lock the page. We avoid that deadlock with
+			 * a trylock_page() and in exchange we risk missing
+			 * some page dirtying.
+			 */
 			set_page_dirty(page);
+			unlock_page(page);
+		}
 
 		mark_page_accessed(page);
 		put_page(page);
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Chris Wilson]

Enable gup to retry and fault the pages outside of the mmap_sem lock in
our worker. As we are inside our worker, outside of any critical path,
we can allow the mmap_sem lock to be dropped in order to service a page
fault; this in turn allows the mm to populate the page using a slow
fault handler.

Testcase: igt/gem_userptr/userfault
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index dd104b0e2071..54ebc7ab71bc 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -459,26 +459,31 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 		unsigned int flags = 0;
+		int locked = 0;
 
 		if (!i915_gem_object_is_readonly(obj))
 			flags |= FOLL_WRITE;
 
 		ret = -EFAULT;
 		if (mmget_not_zero(mm)) {
-			down_read(&mm->mmap_sem);
 			while (pinned < npages) {
+				if (!locked) {
+					down_read(&mm->mmap_sem);
+					locked = 1;
+				}
 				ret = get_user_pages_remote
 					(work->task, mm,
 					 obj->userptr.ptr + pinned * PAGE_SIZE,
 					 npages - pinned,
 					 flags,
-					 pvec + pinned, NULL, NULL);
+					 pvec + pinned, NULL, &locked);
 				if (ret < 0)
 					break;
 
 				pinned += ret;
 			}
-			up_read(&mm->mmap_sem);
+			if (locked)
+				up_read(&mm->mmap_sem);
 			mmput(mm);
 		}
 	}
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Chris Wilson]

Enable gup to retry and fault the pages outside of the mmap_sem lock in
our worker. As we are inside our worker, outside of any critical path,
we can allow the mmap_sem lock to be dropped in order to service a page
fault; this in turn allows the mm to populate the page using a slow
fault handler.

Testcase: igt/gem_userptr/userfault
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
index dd104b0e2071..54ebc7ab71bc 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
@@ -459,26 +459,31 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 		unsigned int flags = 0;
+		int locked = 0;
 
 		if (!i915_gem_object_is_readonly(obj))
 			flags |= FOLL_WRITE;
 
 		ret = -EFAULT;
 		if (mmget_not_zero(mm)) {
-			down_read(&mm->mmap_sem);
 			while (pinned < npages) {
+				if (!locked) {
+					down_read(&mm->mmap_sem);
+					locked = 1;
+				}
 				ret = get_user_pages_remote
 					(work->task, mm,
 					 obj->userptr.ptr + pinned * PAGE_SIZE,
 					 npages - pinned,
 					 flags,
-					 pvec + pinned, NULL, NULL);
+					 pvec + pinned, NULL, &locked);
 				if (ret < 0)
 					break;
 
 				pinned += ret;
 			}
-			up_read(&mm->mmap_sem);
+			if (locked)
+				up_read(&mm->mmap_sem);
 			mmput(mm);
 		}
 	}
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Tvrtko Ursulin]

On 11/11/2019 13:32, Chris Wilson wrote:
> Enable gup to retry and fault the pages outside of the mmap_sem lock in
> our worker. As we are inside our worker, outside of any critical path,
> we can allow the mmap_sem lock to be dropped in order to service a page
> fault; this in turn allows the mm to populate the page using a slow
> fault handler.
> 
> Testcase: igt/gem_userptr/userfault

There are no references or explanation on what does this fix?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index dd104b0e2071..54ebc7ab71bc 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -459,26 +459,31 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	if (pvec != NULL) {
>   		struct mm_struct *mm = obj->userptr.mm->mm;
>   		unsigned int flags = 0;
> +		int locked = 0;
>   
>   		if (!i915_gem_object_is_readonly(obj))
>   			flags |= FOLL_WRITE;
>   
>   		ret = -EFAULT;
>   		if (mmget_not_zero(mm)) {
> -			down_read(&mm->mmap_sem);
>   			while (pinned < npages) {
> +				if (!locked) {
> +					down_read(&mm->mmap_sem);
> +					locked = 1;
> +				}
>   				ret = get_user_pages_remote
>   					(work->task, mm,
>   					 obj->userptr.ptr + pinned * PAGE_SIZE,
>   					 npages - pinned,
>   					 flags,
> -					 pvec + pinned, NULL, NULL);
> +					 pvec + pinned, NULL, &locked);
>   				if (ret < 0)
>   					break;
>   
>   				pinned += ret;
>   			}
> -			up_read(&mm->mmap_sem);
> +			if (locked)
> +				up_read(&mm->mmap_sem);
>   			mmput(mm);
>   		}
>   	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Tvrtko Ursulin]

On 11/11/2019 13:32, Chris Wilson wrote:
> Enable gup to retry and fault the pages outside of the mmap_sem lock in
> our worker. As we are inside our worker, outside of any critical path,
> we can allow the mmap_sem lock to be dropped in order to service a page
> fault; this in turn allows the mm to populate the page using a slow
> fault handler.
> 
> Testcase: igt/gem_userptr/userfault

There are no references or explanation on what does this fix?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index dd104b0e2071..54ebc7ab71bc 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -459,26 +459,31 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
>   	if (pvec != NULL) {
>   		struct mm_struct *mm = obj->userptr.mm->mm;
>   		unsigned int flags = 0;
> +		int locked = 0;
>   
>   		if (!i915_gem_object_is_readonly(obj))
>   			flags |= FOLL_WRITE;
>   
>   		ret = -EFAULT;
>   		if (mmget_not_zero(mm)) {
> -			down_read(&mm->mmap_sem);
>   			while (pinned < npages) {
> +				if (!locked) {
> +					down_read(&mm->mmap_sem);
> +					locked = 1;
> +				}
>   				ret = get_user_pages_remote
>   					(work->task, mm,
>   					 obj->userptr.ptr + pinned * PAGE_SIZE,
>   					 npages - pinned,
>   					 flags,
> -					 pvec + pinned, NULL, NULL);
> +					 pvec + pinned, NULL, &locked);
>   				if (ret < 0)
>   					break;
>   
>   				pinned += ret;
>   			}
> -			up_read(&mm->mmap_sem);
> +			if (locked)
> +				up_read(&mm->mmap_sem);
>   			mmput(mm);
>   		}
>   	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-11 14:19:31)
> 
> On 11/11/2019 13:32, Chris Wilson wrote:
> > Enable gup to retry and fault the pages outside of the mmap_sem lock in
> > our worker. As we are inside our worker, outside of any critical path,
> > we can allow the mmap_sem lock to be dropped in order to service a page
> > fault; this in turn allows the mm to populate the page using a slow
> > fault handler.
> > 
> > Testcase: igt/gem_userptr/userfault
> 
> There are no references or explanation on what does this fix?

gup simply fails if it is not allowed to drop the lock for some faults,

__get_user_pages_locked:
                ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
                                       vmas, locked);
                if (!locked)
                        /* VM_FAULT_RETRY couldn't trigger, bypass */
                        return ret;

userfault being the first time I discovered this even existed. Since we
are only holding the mmap_sem for the gup (and not protecting anything
else) we can simply allow gup to drop the lock if it needs to.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-11 14:19:31)
> 
> On 11/11/2019 13:32, Chris Wilson wrote:
> > Enable gup to retry and fault the pages outside of the mmap_sem lock in
> > our worker. As we are inside our worker, outside of any critical path,
> > we can allow the mmap_sem lock to be dropped in order to service a page
> > fault; this in turn allows the mm to populate the page using a slow
> > fault handler.
> > 
> > Testcase: igt/gem_userptr/userfault
> 
> There are no references or explanation on what does this fix?

gup simply fails if it is not allowed to drop the lock for some faults,

__get_user_pages_locked:
                ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
                                       vmas, locked);
                if (!locked)
                        /* VM_FAULT_RETRY couldn't trigger, bypass */
                        return ret;

userfault being the first time I discovered this even existed. Since we
are only holding the mmap_sem for the gup (and not protecting anything
else) we can simply allow gup to drop the lock if it needs to.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Chris Wilson]

Quoting Chris Wilson (2019-11-11 14:27:16)
> Quoting Tvrtko Ursulin (2019-11-11 14:19:31)
> > 
> > On 11/11/2019 13:32, Chris Wilson wrote:
> > > Enable gup to retry and fault the pages outside of the mmap_sem lock in
> > > our worker. As we are inside our worker, outside of any critical path,
> > > we can allow the mmap_sem lock to be dropped in order to service a page
> > > fault; this in turn allows the mm to populate the page using a slow
> > > fault handler.
> > > 
> > > Testcase: igt/gem_userptr/userfault
> > 
> > There are no references or explanation on what does this fix?
> 
> gup simply fails if it is not allowed to drop the lock for some faults,
> 
> __get_user_pages_locked:
>                 ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
>                                        vmas, locked);
>                 if (!locked)
>                         /* VM_FAULT_RETRY couldn't trigger, bypass */
>                         return ret;
> 
> userfault being the first time I discovered this even existed. Since we
> are only holding the mmap_sem for the gup (and not protecting anything
> else) we can simply allow gup to drop the lock if it needs to.

Fixes: 5b56d49fc31d ("mm: add locked parameter to get_user_pages_remote()")
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Chris Wilson]

Quoting Chris Wilson (2019-11-11 14:27:16)
> Quoting Tvrtko Ursulin (2019-11-11 14:19:31)
> > 
> > On 11/11/2019 13:32, Chris Wilson wrote:
> > > Enable gup to retry and fault the pages outside of the mmap_sem lock in
> > > our worker. As we are inside our worker, outside of any critical path,
> > > we can allow the mmap_sem lock to be dropped in order to service a page
> > > fault; this in turn allows the mm to populate the page using a slow
> > > fault handler.
> > > 
> > > Testcase: igt/gem_userptr/userfault
> > 
> > There are no references or explanation on what does this fix?
> 
> gup simply fails if it is not allowed to drop the lock for some faults,
> 
> __get_user_pages_locked:
>                 ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
>                                        vmas, locked);
>                 if (!locked)
>                         /* VM_FAULT_RETRY couldn't trigger, bypass */
>                         return ret;
> 
> userfault being the first time I discovered this even existed. Since we
> are only holding the mmap_sem for the gup (and not protecting anything
> else) we can simply allow gup to drop the lock if it needs to.

Fixes: 5b56d49fc31d ("mm: add locked parameter to get_user_pages_remote()")
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Tvrtko Ursulin]

On 11/11/2019 14:32, Chris Wilson wrote:
> Quoting Chris Wilson (2019-11-11 14:27:16)
>> Quoting Tvrtko Ursulin (2019-11-11 14:19:31)
>>>
>>> On 11/11/2019 13:32, Chris Wilson wrote:
>>>> Enable gup to retry and fault the pages outside of the mmap_sem lock in
>>>> our worker. As we are inside our worker, outside of any critical path,
>>>> we can allow the mmap_sem lock to be dropped in order to service a page
>>>> fault; this in turn allows the mm to populate the page using a slow
>>>> fault handler.
>>>>
>>>> Testcase: igt/gem_userptr/userfault
>>>
>>> There are no references or explanation on what does this fix?
>>
>> gup simply fails if it is not allowed to drop the lock for some faults,
>>
>> __get_user_pages_locked:
>>                  ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
>>                                         vmas, locked);
>>                  if (!locked)
>>                          /* VM_FAULT_RETRY couldn't trigger, bypass */
>>                          return ret;
>>
>> userfault being the first time I discovered this even existed. Since we
>> are only holding the mmap_sem for the gup (and not protecting anything
>> else) we can simply allow gup to drop the lock if it needs to.
> 
> Fixes: 5b56d49fc31d ("mm: add locked parameter to get_user_pages_remote()")

s/Fixes/Reference/ I guess.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [FIXES 2/3] drm/i915/userptr: Handle unlocked gup retries

[Tvrtko Ursulin]

On 11/11/2019 14:32, Chris Wilson wrote:
> Quoting Chris Wilson (2019-11-11 14:27:16)
>> Quoting Tvrtko Ursulin (2019-11-11 14:19:31)
>>>
>>> On 11/11/2019 13:32, Chris Wilson wrote:
>>>> Enable gup to retry and fault the pages outside of the mmap_sem lock in
>>>> our worker. As we are inside our worker, outside of any critical path,
>>>> we can allow the mmap_sem lock to be dropped in order to service a page
>>>> fault; this in turn allows the mm to populate the page using a slow
>>>> fault handler.
>>>>
>>>> Testcase: igt/gem_userptr/userfault
>>>
>>> There are no references or explanation on what does this fix?
>>
>> gup simply fails if it is not allowed to drop the lock for some faults,
>>
>> __get_user_pages_locked:
>>                  ret = __get_user_pages(tsk, mm, start, nr_pages, flags, pages,
>>                                         vmas, locked);
>>                  if (!locked)
>>                          /* VM_FAULT_RETRY couldn't trigger, bypass */
>>                          return ret;
>>
>> userfault being the first time I discovered this even existed. Since we
>> are only holding the mmap_sem for the gup (and not protecting anything
>> else) we can simply allow gup to drop the lock if it needs to.
> 
> Fixes: 5b56d49fc31d ("mm: add locked parameter to get_user_pages_remote()")

s/Fixes/Reference/ I guess.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[FIXES 3/3] drm/i915/execlists: Move reset_active() from schedule-out to schedule-in

[Chris Wilson]

The gem_ctx_persistence/smoketest was detecting an odd coherency issue
inside the LRC context image; that the address of the ring buffer did
not match our associated struct intel_ring. As we set the address into
the context image when we pin the ring buffer into place before the
context is active, that leaves the question of where did it get
overwritten. Either the HW context save occurred after our pin which
would imply that our idle barriers are broken, or we overwrote the
context image ourselves. It is only in reset_active() where we dabble
inside the context image outside of a serialised path from schedule-out;
but we could equally perform the operation inside schedule-in which is
then fully serialised with the context pin -- and remains serialised by
the engine pulse with kill_context(). (The only downside, aside from
doing more work inside the engine->active.lock, was the plan to merge
all the reset paths into doing their context scrubbing on schedule-out
needs more thought.)

Fixes: d12acee84ffb ("drm/i915/execlists: Cancel banned contexts on schedule-out")
Testcase: igt/gem_ctx_persistence/smoketest
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 114 ++++++++++++++--------------
 1 file changed, 57 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e57345795c08..4b6d9e6b1bfd 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1042,6 +1042,59 @@ execlists_check_context(const struct intel_context *ce,
 	WARN_ONCE(!valid, "Invalid lrc state found before submission\n");
 }
 
+static void restore_default_state(struct intel_context *ce,
+				  struct intel_engine_cs *engine)
+{
+	u32 *regs = ce->lrc_reg_state;
+
+	if (engine->pinned_default_state)
+		memcpy(regs, /* skip restoring the vanilla PPHWSP */
+		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
+		       engine->context_size - PAGE_SIZE);
+
+	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
+}
+
+static void reset_active(struct i915_request *rq,
+			 struct intel_engine_cs *engine)
+{
+	struct intel_context * const ce = rq->hw_context;
+	u32 head;
+
+	/*
+	 * The executing context has been cancelled. We want to prevent
+	 * further execution along this context and propagate the error on
+	 * to anything depending on its results.
+	 *
+	 * In __i915_request_submit(), we apply the -EIO and remove the
+	 * requests' payloads for any banned requests. But first, we must
+	 * rewind the context back to the start of the incomplete request so
+	 * that we do not jump back into the middle of the batch.
+	 *
+	 * We preserve the breadcrumbs and semaphores of the incomplete
+	 * requests so that inter-timeline dependencies (i.e other timelines)
+	 * remain correctly ordered. And we defer to __i915_request_submit()
+	 * so that all asynchronous waits are correctly handled.
+	 */
+	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
+		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
+
+	/* On resubmission of the active request, payload will be scrubbed */
+	if (i915_request_completed(rq))
+		head = rq->tail;
+	else
+		head = active_request(ce->timeline, rq)->head;
+	ce->ring->head = intel_ring_wrap(ce->ring, head);
+	intel_ring_update_space(ce->ring);
+
+	/* Scrub the context image to prevent replaying the previous batch */
+	restore_default_state(ce, engine);
+	__execlists_update_reg_state(ce, engine);
+
+	/* We've switched away, so this should be a no-op, but intent matters */
+	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
+}
+
 static inline struct intel_engine_cs *
 __execlists_schedule_in(struct i915_request *rq)
 {
@@ -1050,8 +1103,11 @@ __execlists_schedule_in(struct i915_request *rq)
 
 	intel_context_get(ce);
 
+	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
+		reset_active(rq, engine);
+
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
-		execlists_check_context(ce, rq->engine);
+		execlists_check_context(ce, engine);
 
 	if (ce->tag) {
 		/* Use a fixed tag for OA and friends */
@@ -1102,59 +1158,6 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
 		tasklet_schedule(&ve->base.execlists.tasklet);
 }
 
-static void restore_default_state(struct intel_context *ce,
-				  struct intel_engine_cs *engine)
-{
-	u32 *regs = ce->lrc_reg_state;
-
-	if (engine->pinned_default_state)
-		memcpy(regs, /* skip restoring the vanilla PPHWSP */
-		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
-		       engine->context_size - PAGE_SIZE);
-
-	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
-}
-
-static void reset_active(struct i915_request *rq,
-			 struct intel_engine_cs *engine)
-{
-	struct intel_context * const ce = rq->hw_context;
-	u32 head;
-
-	/*
-	 * The executing context has been cancelled. We want to prevent
-	 * further execution along this context and propagate the error on
-	 * to anything depending on its results.
-	 *
-	 * In __i915_request_submit(), we apply the -EIO and remove the
-	 * requests' payloads for any banned requests. But first, we must
-	 * rewind the context back to the start of the incomplete request so
-	 * that we do not jump back into the middle of the batch.
-	 *
-	 * We preserve the breadcrumbs and semaphores of the incomplete
-	 * requests so that inter-timeline dependencies (i.e other timelines)
-	 * remain correctly ordered. And we defer to __i915_request_submit()
-	 * so that all asynchronous waits are correctly handled.
-	 */
-	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
-		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
-
-	/* On resubmission of the active request, payload will be scrubbed */
-	if (i915_request_completed(rq))
-		head = rq->tail;
-	else
-		head = active_request(ce->timeline, rq)->head;
-	ce->ring->head = intel_ring_wrap(ce->ring, head);
-	intel_ring_update_space(ce->ring);
-
-	/* Scrub the context image to prevent replaying the previous batch */
-	restore_default_state(ce, engine);
-	__execlists_update_reg_state(ce, engine);
-
-	/* We've switched away, so this should be a no-op, but intent matters */
-	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
-}
-
 static inline void
 __execlists_schedule_out(struct i915_request *rq,
 			 struct intel_engine_cs * const engine)
@@ -1165,9 +1168,6 @@ __execlists_schedule_out(struct i915_request *rq,
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
 	intel_gt_pm_put(engine->gt);
 
-	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
-		reset_active(rq, engine);
-
 	/*
 	 * If this is part of a virtual engine, its next request may
 	 * have been blocked waiting for access to the active context.
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [FIXES 3/3] drm/i915/execlists: Move reset_active() from schedule-out to schedule-in

[Chris Wilson]

The gem_ctx_persistence/smoketest was detecting an odd coherency issue
inside the LRC context image; that the address of the ring buffer did
not match our associated struct intel_ring. As we set the address into
the context image when we pin the ring buffer into place before the
context is active, that leaves the question of where did it get
overwritten. Either the HW context save occurred after our pin which
would imply that our idle barriers are broken, or we overwrote the
context image ourselves. It is only in reset_active() where we dabble
inside the context image outside of a serialised path from schedule-out;
but we could equally perform the operation inside schedule-in which is
then fully serialised with the context pin -- and remains serialised by
the engine pulse with kill_context(). (The only downside, aside from
doing more work inside the engine->active.lock, was the plan to merge
all the reset paths into doing their context scrubbing on schedule-out
needs more thought.)

Fixes: d12acee84ffb ("drm/i915/execlists: Cancel banned contexts on schedule-out")
Testcase: igt/gem_ctx_persistence/smoketest
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_lrc.c | 114 ++++++++++++++--------------
 1 file changed, 57 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e57345795c08..4b6d9e6b1bfd 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -1042,6 +1042,59 @@ execlists_check_context(const struct intel_context *ce,
 	WARN_ONCE(!valid, "Invalid lrc state found before submission\n");
 }
 
+static void restore_default_state(struct intel_context *ce,
+				  struct intel_engine_cs *engine)
+{
+	u32 *regs = ce->lrc_reg_state;
+
+	if (engine->pinned_default_state)
+		memcpy(regs, /* skip restoring the vanilla PPHWSP */
+		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
+		       engine->context_size - PAGE_SIZE);
+
+	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
+}
+
+static void reset_active(struct i915_request *rq,
+			 struct intel_engine_cs *engine)
+{
+	struct intel_context * const ce = rq->hw_context;
+	u32 head;
+
+	/*
+	 * The executing context has been cancelled. We want to prevent
+	 * further execution along this context and propagate the error on
+	 * to anything depending on its results.
+	 *
+	 * In __i915_request_submit(), we apply the -EIO and remove the
+	 * requests' payloads for any banned requests. But first, we must
+	 * rewind the context back to the start of the incomplete request so
+	 * that we do not jump back into the middle of the batch.
+	 *
+	 * We preserve the breadcrumbs and semaphores of the incomplete
+	 * requests so that inter-timeline dependencies (i.e other timelines)
+	 * remain correctly ordered. And we defer to __i915_request_submit()
+	 * so that all asynchronous waits are correctly handled.
+	 */
+	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
+		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
+
+	/* On resubmission of the active request, payload will be scrubbed */
+	if (i915_request_completed(rq))
+		head = rq->tail;
+	else
+		head = active_request(ce->timeline, rq)->head;
+	ce->ring->head = intel_ring_wrap(ce->ring, head);
+	intel_ring_update_space(ce->ring);
+
+	/* Scrub the context image to prevent replaying the previous batch */
+	restore_default_state(ce, engine);
+	__execlists_update_reg_state(ce, engine);
+
+	/* We've switched away, so this should be a no-op, but intent matters */
+	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
+}
+
 static inline struct intel_engine_cs *
 __execlists_schedule_in(struct i915_request *rq)
 {
@@ -1050,8 +1103,11 @@ __execlists_schedule_in(struct i915_request *rq)
 
 	intel_context_get(ce);
 
+	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
+		reset_active(rq, engine);
+
 	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
-		execlists_check_context(ce, rq->engine);
+		execlists_check_context(ce, engine);
 
 	if (ce->tag) {
 		/* Use a fixed tag for OA and friends */
@@ -1102,59 +1158,6 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
 		tasklet_schedule(&ve->base.execlists.tasklet);
 }
 
-static void restore_default_state(struct intel_context *ce,
-				  struct intel_engine_cs *engine)
-{
-	u32 *regs = ce->lrc_reg_state;
-
-	if (engine->pinned_default_state)
-		memcpy(regs, /* skip restoring the vanilla PPHWSP */
-		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
-		       engine->context_size - PAGE_SIZE);
-
-	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
-}
-
-static void reset_active(struct i915_request *rq,
-			 struct intel_engine_cs *engine)
-{
-	struct intel_context * const ce = rq->hw_context;
-	u32 head;
-
-	/*
-	 * The executing context has been cancelled. We want to prevent
-	 * further execution along this context and propagate the error on
-	 * to anything depending on its results.
-	 *
-	 * In __i915_request_submit(), we apply the -EIO and remove the
-	 * requests' payloads for any banned requests. But first, we must
-	 * rewind the context back to the start of the incomplete request so
-	 * that we do not jump back into the middle of the batch.
-	 *
-	 * We preserve the breadcrumbs and semaphores of the incomplete
-	 * requests so that inter-timeline dependencies (i.e other timelines)
-	 * remain correctly ordered. And we defer to __i915_request_submit()
-	 * so that all asynchronous waits are correctly handled.
-	 */
-	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
-		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
-
-	/* On resubmission of the active request, payload will be scrubbed */
-	if (i915_request_completed(rq))
-		head = rq->tail;
-	else
-		head = active_request(ce->timeline, rq)->head;
-	ce->ring->head = intel_ring_wrap(ce->ring, head);
-	intel_ring_update_space(ce->ring);
-
-	/* Scrub the context image to prevent replaying the previous batch */
-	restore_default_state(ce, engine);
-	__execlists_update_reg_state(ce, engine);
-
-	/* We've switched away, so this should be a no-op, but intent matters */
-	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
-}
-
 static inline void
 __execlists_schedule_out(struct i915_request *rq,
 			 struct intel_engine_cs * const engine)
@@ -1165,9 +1168,6 @@ __execlists_schedule_out(struct i915_request *rq,
 	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
 	intel_gt_pm_put(engine->gt);
 
-	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
-		reset_active(rq, engine);
-
 	/*
 	 * If this is part of a virtual engine, its next request may
 	 * have been blocked waiting for access to the active context.
-- 
2.24.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 3/3] drm/i915/execlists: Move reset_active() from schedule-out to schedule-in

[Tvrtko Ursulin]

On 11/11/2019 13:32, Chris Wilson wrote:
> The gem_ctx_persistence/smoketest was detecting an odd coherency issue
> inside the LRC context image; that the address of the ring buffer did
> not match our associated struct intel_ring. As we set the address into
> the context image when we pin the ring buffer into place before the
> context is active, that leaves the question of where did it get
> overwritten. Either the HW context save occurred after our pin which
> would imply that our idle barriers are broken, or we overwrote the
> context image ourselves. It is only in reset_active() where we dabble
> inside the context image outside of a serialised path from schedule-out;
> but we could equally perform the operation inside schedule-in which is
> then fully serialised with the context pin -- and remains serialised by
> the engine pulse with kill_context(). (The only downside, aside from
> doing more work inside the engine->active.lock, was the plan to merge
> all the reset paths into doing their context scrubbing on schedule-out
> needs more thought.)

So process_csb is not under the engine lock and hence schedule_out can 
race with schedule_in meaning schedule_out should refrain from doing 
non-trivial work.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 
> Fixes: d12acee84ffb ("drm/i915/execlists: Cancel banned contexts on schedule-out")
> Testcase: igt/gem_ctx_persistence/smoketest
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 114 ++++++++++++++--------------
>   1 file changed, 57 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index e57345795c08..4b6d9e6b1bfd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1042,6 +1042,59 @@ execlists_check_context(const struct intel_context *ce,
>   	WARN_ONCE(!valid, "Invalid lrc state found before submission\n");
>   }
>   
> +static void restore_default_state(struct intel_context *ce,
> +				  struct intel_engine_cs *engine)
> +{
> +	u32 *regs = ce->lrc_reg_state;
> +
> +	if (engine->pinned_default_state)
> +		memcpy(regs, /* skip restoring the vanilla PPHWSP */
> +		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> +		       engine->context_size - PAGE_SIZE);
> +
> +	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
> +}
> +
> +static void reset_active(struct i915_request *rq,
> +			 struct intel_engine_cs *engine)
> +{
> +	struct intel_context * const ce = rq->hw_context;
> +	u32 head;
> +
> +	/*
> +	 * The executing context has been cancelled. We want to prevent
> +	 * further execution along this context and propagate the error on
> +	 * to anything depending on its results.
> +	 *
> +	 * In __i915_request_submit(), we apply the -EIO and remove the
> +	 * requests' payloads for any banned requests. But first, we must
> +	 * rewind the context back to the start of the incomplete request so
> +	 * that we do not jump back into the middle of the batch.
> +	 *
> +	 * We preserve the breadcrumbs and semaphores of the incomplete
> +	 * requests so that inter-timeline dependencies (i.e other timelines)
> +	 * remain correctly ordered. And we defer to __i915_request_submit()
> +	 * so that all asynchronous waits are correctly handled.
> +	 */
> +	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
> +		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
> +
> +	/* On resubmission of the active request, payload will be scrubbed */
> +	if (i915_request_completed(rq))
> +		head = rq->tail;
> +	else
> +		head = active_request(ce->timeline, rq)->head;
> +	ce->ring->head = intel_ring_wrap(ce->ring, head);
> +	intel_ring_update_space(ce->ring);
> +
> +	/* Scrub the context image to prevent replaying the previous batch */
> +	restore_default_state(ce, engine);
> +	__execlists_update_reg_state(ce, engine);
> +
> +	/* We've switched away, so this should be a no-op, but intent matters */
> +	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
> +}
> +
>   static inline struct intel_engine_cs *
>   __execlists_schedule_in(struct i915_request *rq)
>   {
> @@ -1050,8 +1103,11 @@ __execlists_schedule_in(struct i915_request *rq)
>   
>   	intel_context_get(ce);
>   
> +	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
> +		reset_active(rq, engine);
> +
>   	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> -		execlists_check_context(ce, rq->engine);
> +		execlists_check_context(ce, engine);
>   
>   	if (ce->tag) {
>   		/* Use a fixed tag for OA and friends */
> @@ -1102,59 +1158,6 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
>   		tasklet_schedule(&ve->base.execlists.tasklet);
>   }
>   
> -static void restore_default_state(struct intel_context *ce,
> -				  struct intel_engine_cs *engine)
> -{
> -	u32 *regs = ce->lrc_reg_state;
> -
> -	if (engine->pinned_default_state)
> -		memcpy(regs, /* skip restoring the vanilla PPHWSP */
> -		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> -		       engine->context_size - PAGE_SIZE);
> -
> -	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
> -}
> -
> -static void reset_active(struct i915_request *rq,
> -			 struct intel_engine_cs *engine)
> -{
> -	struct intel_context * const ce = rq->hw_context;
> -	u32 head;
> -
> -	/*
> -	 * The executing context has been cancelled. We want to prevent
> -	 * further execution along this context and propagate the error on
> -	 * to anything depending on its results.
> -	 *
> -	 * In __i915_request_submit(), we apply the -EIO and remove the
> -	 * requests' payloads for any banned requests. But first, we must
> -	 * rewind the context back to the start of the incomplete request so
> -	 * that we do not jump back into the middle of the batch.
> -	 *
> -	 * We preserve the breadcrumbs and semaphores of the incomplete
> -	 * requests so that inter-timeline dependencies (i.e other timelines)
> -	 * remain correctly ordered. And we defer to __i915_request_submit()
> -	 * so that all asynchronous waits are correctly handled.
> -	 */
> -	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
> -		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
> -
> -	/* On resubmission of the active request, payload will be scrubbed */
> -	if (i915_request_completed(rq))
> -		head = rq->tail;
> -	else
> -		head = active_request(ce->timeline, rq)->head;
> -	ce->ring->head = intel_ring_wrap(ce->ring, head);
> -	intel_ring_update_space(ce->ring);
> -
> -	/* Scrub the context image to prevent replaying the previous batch */
> -	restore_default_state(ce, engine);
> -	__execlists_update_reg_state(ce, engine);
> -
> -	/* We've switched away, so this should be a no-op, but intent matters */
> -	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
> -}
> -
>   static inline void
>   __execlists_schedule_out(struct i915_request *rq,
>   			 struct intel_engine_cs * const engine)
> @@ -1165,9 +1168,6 @@ __execlists_schedule_out(struct i915_request *rq,
>   	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
>   	intel_gt_pm_put(engine->gt);
>   
> -	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
> -		reset_active(rq, engine);
> -
>   	/*
>   	 * If this is part of a virtual engine, its next request may
>   	 * have been blocked waiting for access to the active context.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [FIXES 3/3] drm/i915/execlists: Move reset_active() from schedule-out to schedule-in

[Tvrtko Ursulin]

On 11/11/2019 13:32, Chris Wilson wrote:
> The gem_ctx_persistence/smoketest was detecting an odd coherency issue
> inside the LRC context image; that the address of the ring buffer did
> not match our associated struct intel_ring. As we set the address into
> the context image when we pin the ring buffer into place before the
> context is active, that leaves the question of where did it get
> overwritten. Either the HW context save occurred after our pin which
> would imply that our idle barriers are broken, or we overwrote the
> context image ourselves. It is only in reset_active() where we dabble
> inside the context image outside of a serialised path from schedule-out;
> but we could equally perform the operation inside schedule-in which is
> then fully serialised with the context pin -- and remains serialised by
> the engine pulse with kill_context(). (The only downside, aside from
> doing more work inside the engine->active.lock, was the plan to merge
> all the reset paths into doing their context scrubbing on schedule-out
> needs more thought.)

So process_csb is not under the engine lock and hence schedule_out can 
race with schedule_in meaning schedule_out should refrain from doing 
non-trivial work.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 
> Fixes: d12acee84ffb ("drm/i915/execlists: Cancel banned contexts on schedule-out")
> Testcase: igt/gem_ctx_persistence/smoketest
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_lrc.c | 114 ++++++++++++++--------------
>   1 file changed, 57 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index e57345795c08..4b6d9e6b1bfd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1042,6 +1042,59 @@ execlists_check_context(const struct intel_context *ce,
>   	WARN_ONCE(!valid, "Invalid lrc state found before submission\n");
>   }
>   
> +static void restore_default_state(struct intel_context *ce,
> +				  struct intel_engine_cs *engine)
> +{
> +	u32 *regs = ce->lrc_reg_state;
> +
> +	if (engine->pinned_default_state)
> +		memcpy(regs, /* skip restoring the vanilla PPHWSP */
> +		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> +		       engine->context_size - PAGE_SIZE);
> +
> +	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
> +}
> +
> +static void reset_active(struct i915_request *rq,
> +			 struct intel_engine_cs *engine)
> +{
> +	struct intel_context * const ce = rq->hw_context;
> +	u32 head;
> +
> +	/*
> +	 * The executing context has been cancelled. We want to prevent
> +	 * further execution along this context and propagate the error on
> +	 * to anything depending on its results.
> +	 *
> +	 * In __i915_request_submit(), we apply the -EIO and remove the
> +	 * requests' payloads for any banned requests. But first, we must
> +	 * rewind the context back to the start of the incomplete request so
> +	 * that we do not jump back into the middle of the batch.
> +	 *
> +	 * We preserve the breadcrumbs and semaphores of the incomplete
> +	 * requests so that inter-timeline dependencies (i.e other timelines)
> +	 * remain correctly ordered. And we defer to __i915_request_submit()
> +	 * so that all asynchronous waits are correctly handled.
> +	 */
> +	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
> +		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
> +
> +	/* On resubmission of the active request, payload will be scrubbed */
> +	if (i915_request_completed(rq))
> +		head = rq->tail;
> +	else
> +		head = active_request(ce->timeline, rq)->head;
> +	ce->ring->head = intel_ring_wrap(ce->ring, head);
> +	intel_ring_update_space(ce->ring);
> +
> +	/* Scrub the context image to prevent replaying the previous batch */
> +	restore_default_state(ce, engine);
> +	__execlists_update_reg_state(ce, engine);
> +
> +	/* We've switched away, so this should be a no-op, but intent matters */
> +	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
> +}
> +
>   static inline struct intel_engine_cs *
>   __execlists_schedule_in(struct i915_request *rq)
>   {
> @@ -1050,8 +1103,11 @@ __execlists_schedule_in(struct i915_request *rq)
>   
>   	intel_context_get(ce);
>   
> +	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
> +		reset_active(rq, engine);
> +
>   	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> -		execlists_check_context(ce, rq->engine);
> +		execlists_check_context(ce, engine);
>   
>   	if (ce->tag) {
>   		/* Use a fixed tag for OA and friends */
> @@ -1102,59 +1158,6 @@ static void kick_siblings(struct i915_request *rq, struct intel_context *ce)
>   		tasklet_schedule(&ve->base.execlists.tasklet);
>   }
>   
> -static void restore_default_state(struct intel_context *ce,
> -				  struct intel_engine_cs *engine)
> -{
> -	u32 *regs = ce->lrc_reg_state;
> -
> -	if (engine->pinned_default_state)
> -		memcpy(regs, /* skip restoring the vanilla PPHWSP */
> -		       engine->pinned_default_state + LRC_STATE_PN * PAGE_SIZE,
> -		       engine->context_size - PAGE_SIZE);
> -
> -	execlists_init_reg_state(regs, ce, engine, ce->ring, false);
> -}
> -
> -static void reset_active(struct i915_request *rq,
> -			 struct intel_engine_cs *engine)
> -{
> -	struct intel_context * const ce = rq->hw_context;
> -	u32 head;
> -
> -	/*
> -	 * The executing context has been cancelled. We want to prevent
> -	 * further execution along this context and propagate the error on
> -	 * to anything depending on its results.
> -	 *
> -	 * In __i915_request_submit(), we apply the -EIO and remove the
> -	 * requests' payloads for any banned requests. But first, we must
> -	 * rewind the context back to the start of the incomplete request so
> -	 * that we do not jump back into the middle of the batch.
> -	 *
> -	 * We preserve the breadcrumbs and semaphores of the incomplete
> -	 * requests so that inter-timeline dependencies (i.e other timelines)
> -	 * remain correctly ordered. And we defer to __i915_request_submit()
> -	 * so that all asynchronous waits are correctly handled.
> -	 */
> -	GEM_TRACE("%s(%s): { rq=%llx:%lld }\n",
> -		  __func__, engine->name, rq->fence.context, rq->fence.seqno);
> -
> -	/* On resubmission of the active request, payload will be scrubbed */
> -	if (i915_request_completed(rq))
> -		head = rq->tail;
> -	else
> -		head = active_request(ce->timeline, rq)->head;
> -	ce->ring->head = intel_ring_wrap(ce->ring, head);
> -	intel_ring_update_space(ce->ring);
> -
> -	/* Scrub the context image to prevent replaying the previous batch */
> -	restore_default_state(ce, engine);
> -	__execlists_update_reg_state(ce, engine);
> -
> -	/* We've switched away, so this should be a no-op, but intent matters */
> -	ce->lrc_desc |= CTX_DESC_FORCE_RESTORE;
> -}
> -
>   static inline void
>   __execlists_schedule_out(struct i915_request *rq,
>   			 struct intel_engine_cs * const engine)
> @@ -1165,9 +1168,6 @@ __execlists_schedule_out(struct i915_request *rq,
>   	execlists_context_status_change(rq, INTEL_CONTEXT_SCHEDULE_OUT);
>   	intel_gt_pm_put(engine->gt);
>   
> -	if (unlikely(i915_gem_context_is_banned(ce->gem_context)))
> -		reset_active(rq, engine);
> -
>   	/*
>   	 * If this is part of a virtual engine, its next request may
>   	 * have been blocked waiting for access to the active context.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 3/3] drm/i915/execlists: Move reset_active() from schedule-out to schedule-in

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-11 16:31:09)
> 
> On 11/11/2019 13:32, Chris Wilson wrote:
> > The gem_ctx_persistence/smoketest was detecting an odd coherency issue
> > inside the LRC context image; that the address of the ring buffer did
> > not match our associated struct intel_ring. As we set the address into
> > the context image when we pin the ring buffer into place before the
> > context is active, that leaves the question of where did it get
> > overwritten. Either the HW context save occurred after our pin which
> > would imply that our idle barriers are broken, or we overwrote the
> > context image ourselves. It is only in reset_active() where we dabble
> > inside the context image outside of a serialised path from schedule-out;
> > but we could equally perform the operation inside schedule-in which is
> > then fully serialised with the context pin -- and remains serialised by
> > the engine pulse with kill_context(). (The only downside, aside from
> > doing more work inside the engine->active.lock, was the plan to merge
> > all the reset paths into doing their context scrubbing on schedule-out
> > needs more thought.)
> 
> So process_csb is not under the engine lock and hence schedule_out can 
> race with schedule_in meaning schedule_out should refrain from doing 
> non-trivial work.

I'm stealing that for a comment in schedule_out! Thanks,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [FIXES 3/3] drm/i915/execlists: Move reset_active() from schedule-out to schedule-in

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-11-11 16:31:09)
> 
> On 11/11/2019 13:32, Chris Wilson wrote:
> > The gem_ctx_persistence/smoketest was detecting an odd coherency issue
> > inside the LRC context image; that the address of the ring buffer did
> > not match our associated struct intel_ring. As we set the address into
> > the context image when we pin the ring buffer into place before the
> > context is active, that leaves the question of where did it get
> > overwritten. Either the HW context save occurred after our pin which
> > would imply that our idle barriers are broken, or we overwrote the
> > context image ourselves. It is only in reset_active() where we dabble
> > inside the context image outside of a serialised path from schedule-out;
> > but we could equally perform the operation inside schedule-in which is
> > then fully serialised with the context pin -- and remains serialised by
> > the engine pulse with kill_context(). (The only downside, aside from
> > doing more work inside the engine->active.lock, was the plan to merge
> > all the reset paths into doing their context scrubbing on schedule-out
> > needs more thought.)
> 
> So process_csb is not under the engine lock and hence schedule_out can 
> race with schedule_in meaning schedule_out should refrain from doing 
> non-trivial work.

I'm stealing that for a comment in schedule_out! Thanks,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [FIXES 1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[Tvrtko Ursulin]

On 11/11/2019 13:32, Chris Wilson wrote:
> set_page_dirty says:
> 
> 	For pages with a mapping this should be done under the page lock
> 	for the benefit of asynchronous memory errors who prefer a
> 	consistent dirty state. This rule can be broken in some special
> 	cases, but should be better not to.
> 
> Under those rules, it is only safe for us to use the plain set_page_dirty
> calls for shmemfs/anonymous memory. Userptr may be used with real
> mappings and so needs to use the locked version (set_page_dirty_lock).
> 
> However, following a try_to_unmap() we may want to remove the userptr and
> so call put_pages(). However, try_to_unmap() acquires the page lock and
> so we must avoid recursively locking the pages ourselves -- which means
> that we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203317
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112012
> Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video m
> References: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> References: 505a8ec7e11a ("Revert "drm/i915/userptr: Acquire the page lock around set_page_dirty()"")
> References: 6dcc693bc57f ("ext4: warn when page is dirtied without buffers")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 22 ++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index ee65c6acf0e2..dd104b0e2071 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -646,8 +646,28 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   		obj->mm.dirty = false;
>   
>   	for_each_sgt_page(page, sgt_iter, pages) {
> -		if (obj->mm.dirty)
> +		if (obj->mm.dirty && trylock_page(page)) {
> +			/*
> +			 * As this may not be anonymous memory (e.g. shmem)
> +			 * but exist on a real mapping, we have to lock
> +			 * the page in order to dirty it -- holding
> +			 * the page reference is not sufficient to
> +			 * prevent the inode from being truncated.
> +			 * Play safe and take the lock.
> +			 *
> +			 * However...!
> +			 *
> +			 * The mmu-notifier can be invalidated for a
> +			 * migrate_page, that is alreadying holding the lock
> +			 * on the page. Such a try_to_unmap() will result
> +			 * in us calling put_pages() and so recursively try
> +			 * to lock the page. We avoid that deadlock with
> +			 * a trylock_page() and in exchange we risk missing
> +			 * some page dirtying.
> +			 */
>   			set_page_dirty(page);
> +			unlock_page(page);
> +		}
>   
>   		mark_page_accessed(page);
>   		put_page(page);
> 

It looks that the bug report could be about BUG_ON(PageWriteback(page)) 
in ext4/mpage_prepare_extent_to_map which would be somewhat consistent 
with not being allowed to call set_page_dirty on an unlocked page. So on 
the basis of that:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Re: [Intel-gfx] [FIXES 1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[Tvrtko Ursulin]

On 11/11/2019 13:32, Chris Wilson wrote:
> set_page_dirty says:
> 
> 	For pages with a mapping this should be done under the page lock
> 	for the benefit of asynchronous memory errors who prefer a
> 	consistent dirty state. This rule can be broken in some special
> 	cases, but should be better not to.
> 
> Under those rules, it is only safe for us to use the plain set_page_dirty
> calls for shmemfs/anonymous memory. Userptr may be used with real
> mappings and so needs to use the locked version (set_page_dirty_lock).
> 
> However, following a try_to_unmap() we may want to remove the userptr and
> so call put_pages(). However, try_to_unmap() acquires the page lock and
> so we must avoid recursively locking the pages ourselves -- which means
> that we cannot safely acquire the lock around set_page_dirty(). Since we
> can't be sure of the lock, we have to risk skip dirtying the page, or
> else risk calling set_page_dirty() without a lock and so risk fs
> corruption.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=203317
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112012
> Fixes: 5cc9ed4b9a7a ("drm/i915: Introduce mapping of user pages into video m
> References: cb6d7c7dc7ff ("drm/i915/userptr: Acquire the page lock around set_page_dirty()")
> References: 505a8ec7e11a ("Revert "drm/i915/userptr: Acquire the page lock around set_page_dirty()"")
> References: 6dcc693bc57f ("ext4: warn when page is dirtied without buffers")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 22 ++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> index ee65c6acf0e2..dd104b0e2071 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c
> @@ -646,8 +646,28 @@ i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj,
>   		obj->mm.dirty = false;
>   
>   	for_each_sgt_page(page, sgt_iter, pages) {
> -		if (obj->mm.dirty)
> +		if (obj->mm.dirty && trylock_page(page)) {
> +			/*
> +			 * As this may not be anonymous memory (e.g. shmem)
> +			 * but exist on a real mapping, we have to lock
> +			 * the page in order to dirty it -- holding
> +			 * the page reference is not sufficient to
> +			 * prevent the inode from being truncated.
> +			 * Play safe and take the lock.
> +			 *
> +			 * However...!
> +			 *
> +			 * The mmu-notifier can be invalidated for a
> +			 * migrate_page, that is alreadying holding the lock
> +			 * on the page. Such a try_to_unmap() will result
> +			 * in us calling put_pages() and so recursively try
> +			 * to lock the page. We avoid that deadlock with
> +			 * a trylock_page() and in exchange we risk missing
> +			 * some page dirtying.
> +			 */
>   			set_page_dirty(page);
> +			unlock_page(page);
> +		}
>   
>   		mark_page_accessed(page);
>   		put_page(page);
> 

It looks that the bug report could be about BUG_ON(PageWriteback(page)) 
in ext4/mpage_prepare_extent_to_map which would be somewhat consistent 
with not being allowed to call set_page_dirty on an unlocked page. So on 
the basis of that:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [FIXES,1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[Patchwork]

== Series Details ==

Series: series starting with [FIXES,1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()
URL   : https://patchwork.freedesktop.org/series/69296/
State : failure

== Summary ==

Applying: drm/i915/userptr: Try to acquire the page lock around set_page_dirty()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_userptr.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_userptr.c
No changes -- Patch already applied.
Applying: drm/i915/userptr: Handle unlocked gup retries
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_userptr.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915/execlists: Move reset_active() from schedule-out to schedule-in
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gt/intel_lrc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gt/intel_lrc.c
No changes -- Patch already applied.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [FIXES,1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()

[Patchwork]

== Series Details ==

Series: series starting with [FIXES,1/3] drm/i915/userptr: Try to acquire the page lock around set_page_dirty()
URL   : https://patchwork.freedesktop.org/series/69296/
State : failure

== Summary ==

Applying: drm/i915/userptr: Try to acquire the page lock around set_page_dirty()
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_userptr.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_userptr.c
No changes -- Patch already applied.
Applying: drm/i915/userptr: Handle unlocked gup retries
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/i915_gem_userptr.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: drm/i915/execlists: Move reset_active() from schedule-out to schedule-in
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gt/intel_lrc.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gt/intel_lrc.c
No changes -- Patch already applied.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx