Raciness with page table shadows being swapped out

Submitted by Nicolai Hähnle on Dec. 12, 2016, 3:20 p.m.

Details

Message ID 9153dde6-9087-94b0-a062-dd223e5f381a@gmail.com
State New
Headers show
Series "Raciness with page table shadows being swapped out" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Nicolai Hähnle Dec. 12, 2016, 3:20 p.m.
Hi all,

I just sent out two patches that hopefully make the kernel module more 
robust in the face of page table shadows being swapped out.

However, even with those patches, I can still fairly reliably reproduce 
crashes with a backtrace of the shape

amdgpu_cs_ioctl
  -> amdgpu_vm_update_page_directory
  -> amdgpu_ttm_bind
  -> amdgpu_gtt_mgr_alloc

The plausible reason for these crashes is that nothing seems to prevent 
the shadow BOs from being moved between the calls to amdgpu_cs_validate 
in amdgpu_cs_parser_bos and the calls to amdgpu_ttm_bind.

The attached patch has fixed these crashes for me so far, but it's very 
heavy-handed: it collects all page table shadows and the page directory 
shadow and adds them all to the reservations for the callers of 
amdgpu_vm_update_page_directory.

I feel like there should be a better way. In part, I wonder why the 
shadows are needed in the first place. I vaguely recall the discussions 
about GPU reset and such, but I don't remember why the page tables can't 
just be rebuilt in some other way.

Cheers,
Nicolai

Patch hide | download patch | download mbox

From 70706d2283773f19b42312bc34e36c4717a9ce62 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicolai=20H=C3=A4hnle?= <nicolai.haehnle@amd.com>
Date: Mon, 12 Dec 2016 15:02:27 +0100
Subject: [PATCH] drm/amd/amdgpu: reserve shadows of page directory and tables
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  9 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 10 +++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 45 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  3 +++
 5 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index f53e52f..7a84648 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -916,20 +916,21 @@  struct amdgpu_cs_parser {
 	unsigned		nchunks;
 	struct amdgpu_cs_chunk	*chunks;
 
 	/* scheduler job object */
 	struct amdgpu_job	*job;
 
 	/* buffer objects */
 	struct ww_acquire_ctx		ticket;
 	struct amdgpu_bo_list		*bo_list;
 	struct amdgpu_bo_list_entry	vm_pd;
+	struct amdgpu_bo_list_entry	*vm_pd_pt_shadows;
 	struct list_head		validated;
 	struct dma_fence		*fence;
 	uint64_t			bytes_moved_threshold;
 	uint64_t			bytes_moved;
 	struct amdgpu_bo_list_entry	*evictable;
 
 	/* user fence */
 	struct amdgpu_bo_list_entry	uf_entry;
 };
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 985f842..eecf2eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -510,20 +510,26 @@  static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	p->bo_list = amdgpu_bo_list_get(fpriv, cs->in.bo_list_handle);
 	if (p->bo_list) {
 		need_mmap_lock = p->bo_list->first_userptr !=
 			p->bo_list->num_entries;
 		amdgpu_bo_list_get_list(p->bo_list, &p->validated);
 	}
 
 	INIT_LIST_HEAD(&duplicates);
 	amdgpu_vm_get_pd_bo(&fpriv->vm, &p->validated, &p->vm_pd);
 
+	r = amdgpu_vm_get_pd_pt_shadows(&fpriv->vm, &p->validated, &p->vm_pd_pt_shadows);
+	if (r) {
+		DRM_ERROR("amdgpu_vm_get_pd_pt_shadows failed.\n");
+		goto error_release_list;
+	}
+
 	if (p->uf_entry.robj)
 		list_add(&p->uf_entry.tv.head, &p->validated);
 
 	if (need_mmap_lock)
 		down_read(&current->mm->mmap_sem);
 
 	while (1) {
 		struct list_head need_pages;
 		unsigned i;
 
@@ -673,20 +679,21 @@  static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
 	if (r) {
 		amdgpu_vm_move_pt_bos_in_lru(p->adev, &fpriv->vm);
 		ttm_eu_backoff_reservation(&p->ticket, &p->validated);
 	}
 
 error_free_pages:
 
 	if (need_mmap_lock)
 		up_read(&current->mm->mmap_sem);
 
+error_release_list:
 	if (p->bo_list) {
 		for (i = p->bo_list->first_userptr;
 		     i < p->bo_list->num_entries; ++i) {
 			e = &p->bo_list->array[i];
 
 			if (!e->user_pages)
 				continue;
 
 			release_pages(e->user_pages,
 				      e->robj->tbo.ttm->num_pages,
@@ -736,20 +743,22 @@  static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error, bo
 		ttm_eu_backoff_reservation(&parser->ticket,
 					   &parser->validated);
 	}
 	dma_fence_put(parser->fence);
 
 	if (parser->ctx)
 		amdgpu_ctx_put(parser->ctx);
 	if (parser->bo_list)
 		amdgpu_bo_list_put(parser->bo_list);
 
+	drm_free_large(parser->vm_pd_pt_shadows);
+
 	for (i = 0; i < parser->nchunks; i++)
 		drm_free_large(parser->chunks[i].kdata);
 	kfree(parser->chunks);
 	if (parser->job)
 		amdgpu_job_free(parser->job);
 	amdgpu_bo_unref(&parser->uf_entry.robj);
 }
 
 static int amdgpu_bo_vm_update_pte(struct amdgpu_cs_parser *p,
 				   struct amdgpu_vm *vm)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index ffb3e70..379ccde 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -495,39 +495,44 @@  static int amdgpu_gem_va_check(void *param, struct amdgpu_bo *bo)
  *
  * Update the bo_va directly after setting it's address. Errors are not
  * vital here, so they are not reported back to userspace.
  */
 static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 				    struct amdgpu_bo_va *bo_va,
 				    uint32_t operation)
 {
 	struct ttm_validate_buffer tv, *entry;
 	struct amdgpu_bo_list_entry vm_pd;
+	struct amdgpu_bo_list_entry *vm_pd_pt_shadows;
 	struct amdgpu_bo *pd_shadow;
 	struct ww_acquire_ctx ticket;
 	struct list_head list, duplicates;
 	unsigned domain;
 	int r;
 
 	INIT_LIST_HEAD(&list);
 	INIT_LIST_HEAD(&duplicates);
 
 	tv.bo = &bo_va->bo->tbo;
 	tv.shared = true;
 	list_add(&tv.head, &list);
 
 	amdgpu_vm_get_pd_bo(bo_va->vm, &list, &vm_pd);
 
+	r = amdgpu_vm_get_pd_pt_shadows(bo_va->vm, &list, &vm_pd_pt_shadows);
+	if (r)
+		goto error_print;
+
 	/* Provide duplicates to avoid -EALREADY */
 	r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
 	if (r)
-		goto error_print;
+		goto error_reserve_failed;
 
 	list_for_each_entry(entry, &list, head) {
 		domain = amdgpu_mem_type_to_domain(entry->bo->mem.mem_type);
 		/* if anything is swapped out don't swap it in here,
 		   just abort and wait for the next CS */
 		if (domain == AMDGPU_GEM_DOMAIN_CPU)
 			goto error_unreserve;
 	}
 
 	/* Also abort if the page directory shadow has been swapped out. */
@@ -551,20 +556,23 @@  static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
 	r = amdgpu_vm_clear_freed(adev, bo_va->vm);
 	if (r)
 		goto error_unreserve;
 
 	if (operation == AMDGPU_VA_OP_MAP)
 		r = amdgpu_vm_bo_update(adev, bo_va, false);
 
 error_unreserve:
 	ttm_eu_backoff_reservation(&ticket, &list);
 
+error_reserve_failed:
+	drm_free_large(vm_pd_pt_shadows);
+
 error_print:
 	if (r && r != -ERESTARTSYS)
 		DRM_ERROR("Couldn't update BO_VA (%d)\n", r);
 }
 
 int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 			  struct drm_file *filp)
 {
 	struct drm_amdgpu_gem_va *args = data;
 	struct drm_gem_object *gobj;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 1dda932..b63a9bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -109,20 +109,65 @@  void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 {
 	entry->robj = vm->page_directory;
 	entry->priority = 0;
 	entry->tv.bo = &vm->page_directory->tbo;
 	entry->tv.shared = true;
 	entry->user_pages = NULL;
 	list_add(&entry->tv.head, validated);
 }
 
 /**
+ * TODO document
+ */
+int amdgpu_vm_get_pd_pt_shadows(struct amdgpu_vm *vm,
+				struct list_head *validated,
+				struct amdgpu_bo_list_entry **pentries)
+{
+	struct amdgpu_bo *shadow;
+	struct amdgpu_bo_list_entry *entry;
+	unsigned nentries;
+	unsigned i;
+
+	*pentries = drm_calloc_large(vm->max_pde_used + 2,
+				     sizeof(struct amdgpu_bo_list_entry));
+	if (!*pentries)
+		return -ENOMEM;
+
+	nentries = 0;
+
+	if (vm->page_directory->shadow) {
+		shadow = vm->page_directory->shadow;
+		entry = &(*pentries)[nentries++];
+		entry->robj = shadow;
+		entry->tv.bo = &shadow->tbo;
+		entry->tv.shared = true;
+		list_add(&entry->tv.head, validated);
+	}
+
+	for (i = 0; i <= vm->max_pde_used; ++i) {
+		struct amdgpu_bo *bo = vm->page_tables[i].bo;
+
+		if (!bo || !bo->shadow)
+			continue;
+
+		shadow = bo->shadow;
+		entry = &(*pentries)[nentries++];
+		entry->robj = shadow;
+		entry->tv.bo = &shadow->tbo;
+		entry->tv.shared = true;
+		list_add(&entry->tv.head, validated);
+	}
+
+	return 0;
+}
+
+/**
  * amdgpu_vm_validate_pt_bos - validate the page table BOs
  *
  * @adev: amdgpu device pointer
  * @vm: vm providing the BOs
  * @validate: callback to do the validation
  * @param: parameter for the validation callback
  *
  * Validate the page table BOs on command submission if neccessary.
  */
 int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index adbc2f5..ad0b87a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -159,20 +159,23 @@  struct amdgpu_vm_manager {
 	atomic64_t				client_counter;
 };
 
 void amdgpu_vm_manager_init(struct amdgpu_device *adev);
 void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
 int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
 void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
 			 struct list_head *validated,
 			 struct amdgpu_bo_list_entry *entry);
+int amdgpu_vm_get_pd_pt_shadows(struct amdgpu_vm *vm,
+				struct list_head *validated,
+				struct amdgpu_bo_list_entry **pentries);
 int amdgpu_vm_validate_pt_bos(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 			      int (*callback)(void *p, struct amdgpu_bo *bo),
 			      void *param);
 void amdgpu_vm_move_pt_bos_in_lru(struct amdgpu_device *adev,
 				  struct amdgpu_vm *vm);
 int amdgpu_vm_grab_id(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 		      struct amdgpu_sync *sync, struct dma_fence *fence,
 		      struct amdgpu_job *job);
 int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job);
 void amdgpu_vm_reset_id(struct amdgpu_device *adev, unsigned vm_id);
-- 
2.7.4


Comments

Am 12.12.2016 um 16:20 schrieb Nicolai Hähnle:
> Hi all,
>
> I just sent out two patches that hopefully make the kernel module more 
> robust in the face of page table shadows being swapped out.
>
> However, even with those patches, I can still fairly reliably 
> reproduce crashes with a backtrace of the shape
>
> amdgpu_cs_ioctl
>  -> amdgpu_vm_update_page_directory
>  -> amdgpu_ttm_bind
>  -> amdgpu_gtt_mgr_alloc
>
> The plausible reason for these crashes is that nothing seems to 
> prevent the shadow BOs from being moved between the calls to 
> amdgpu_cs_validate in amdgpu_cs_parser_bos and the calls to 
> amdgpu_ttm_bind.

The shadow BOs use the same reservation object than the real BOs. So as 
long as the real BOs can't be evicted the shadows can't be evicted either.

>
> The attached patch has fixed these crashes for me so far, but it's 
> very heavy-handed: it collects all page table shadows and the page 
> directory shadow and adds them all to the reservations for the callers 
> of amdgpu_vm_update_page_directory.

That is most likely just a timing change, cause the shadows should end 
up in the duplicates list anyway. So the patch shouldn't have any effect.

>
> I feel like there should be a better way. In part, I wonder why the 
> shadows are needed in the first place. I vaguely recall the 
> discussions about GPU reset and such, but I don't remember why the 
> page tables can't just be rebuilt in some other way.

It's just the simplest and fastest way to keep a copy of the page tables 
around.

The problem with rebuilding the page tables from the mappings is that 
the housekeeping structures already have the future state when a reset 
happens, not the state we need to rebuild the tables.

We could obviously change the housekeeping a bit to keep both states, 
but that would complicate mapping and unmapping of BOs significantly.

Regards,
Christian.

>
> Cheers,
> Nicolai
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 12.12.2016 19:22, Christian König wrote:
> Am 12.12.2016 um 16:20 schrieb Nicolai Hähnle:
>> Hi all,
>>
>> I just sent out two patches that hopefully make the kernel module more
>> robust in the face of page table shadows being swapped out.
>>
>> However, even with those patches, I can still fairly reliably
>> reproduce crashes with a backtrace of the shape
>>
>> amdgpu_cs_ioctl
>>  -> amdgpu_vm_update_page_directory
>>  -> amdgpu_ttm_bind
>>  -> amdgpu_gtt_mgr_alloc
>>
>> The plausible reason for these crashes is that nothing seems to
>> prevent the shadow BOs from being moved between the calls to
>> amdgpu_cs_validate in amdgpu_cs_parser_bos and the calls to
>> amdgpu_ttm_bind.
>
> The shadow BOs use the same reservation object than the real BOs. So as
> long as the real BOs can't be evicted the shadows can't be evicted either.

Ah okay. I don't see where the page table BOs are added to the buffer 
list. Do they also share the reservation object with the page directory?


>> The attached patch has fixed these crashes for me so far, but it's
>> very heavy-handed: it collects all page table shadows and the page
>> directory shadow and adds them all to the reservations for the callers
>> of amdgpu_vm_update_page_directory.
>
> That is most likely just a timing change, cause the shadows should end
> up in the duplicates list anyway. So the patch shouldn't have any effect.

Okay, so the reason for the remaining crash is still unclear at least 
for me.

Cheers,
Nicolai

>
>>
>> I feel like there should be a better way. In part, I wonder why the
>> shadows are needed in the first place. I vaguely recall the
>> discussions about GPU reset and such, but I don't remember why the
>> page tables can't just be rebuilt in some other way.
>
> It's just the simplest and fastest way to keep a copy of the page tables
> around.
>
> The problem with rebuilding the page tables from the mappings is that
> the housekeeping structures already have the future state when a reset
> happens, not the state we need to rebuild the tables.
>
> We could obviously change the housekeeping a bit to keep both states,
> but that would complicate mapping and unmapping of BOs significantly.
>
> Regards,
> Christian.
>
>>
>> Cheers,
>> Nicolai
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
Am 13.12.2016 um 00:35 schrieb Nicolai Hähnle:
> On 12.12.2016 19:22, Christian König wrote:
>> Am 12.12.2016 um 16:20 schrieb Nicolai Hähnle:
>>> Hi all,
>>>
>>> I just sent out two patches that hopefully make the kernel module more
>>> robust in the face of page table shadows being swapped out.
>>>
>>> However, even with those patches, I can still fairly reliably
>>> reproduce crashes with a backtrace of the shape
>>>
>>> amdgpu_cs_ioctl
>>>  -> amdgpu_vm_update_page_directory
>>>  -> amdgpu_ttm_bind
>>>  -> amdgpu_gtt_mgr_alloc
>>>
>>> The plausible reason for these crashes is that nothing seems to
>>> prevent the shadow BOs from being moved between the calls to
>>> amdgpu_cs_validate in amdgpu_cs_parser_bos and the calls to
>>> amdgpu_ttm_bind.
>>
>> The shadow BOs use the same reservation object than the real BOs. So as
>> long as the real BOs can't be evicted the shadows can't be evicted 
>> either.
>
> Ah okay. I don't see where the page table BOs are added to the buffer 
> list. Do they also share the reservation object with the page directory?

Yes, exactly. All use the same reservation object, so when you hold the 
lock on the PD nothing in the VM will be evicted any more.

>
>
>>> The attached patch has fixed these crashes for me so far, but it's
>>> very heavy-handed: it collects all page table shadows and the page
>>> directory shadow and adds them all to the reservations for the callers
>>> of amdgpu_vm_update_page_directory.
>>
>> That is most likely just a timing change, cause the shadows should end
>> up in the duplicates list anyway. So the patch shouldn't have any 
>> effect.
>
> Okay, so the reason for the remaining crash is still unclear at least 
> for me.

Yeah, that's a really good question. Can you share the call stack of the 
problem once more?

Thanks,
Christian.

>
> Cheers,
> Nicolai
>
>>
>>>
>>> I feel like there should be a better way. In part, I wonder why the
>>> shadows are needed in the first place. I vaguely recall the
>>> discussions about GPU reset and such, but I don't remember why the
>>> page tables can't just be rebuilt in some other way.
>>
>> It's just the simplest and fastest way to keep a copy of the page tables
>> around.
>>
>> The problem with rebuilding the page tables from the mappings is that
>> the housekeeping structures already have the future state when a reset
>> happens, not the state we need to rebuild the tables.
>>
>> We could obviously change the housekeeping a bit to keep both states,
>> but that would complicate mapping and unmapping of BOs significantly.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Cheers,
>>> Nicolai
>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
On 13.12.2016 10:48, Christian König wrote:
>>>> The attached patch has fixed these crashes for me so far, but it's
>>>> very heavy-handed: it collects all page table shadows and the page
>>>> directory shadow and adds them all to the reservations for the callers
>>>> of amdgpu_vm_update_page_directory.
>>>
>>> That is most likely just a timing change, cause the shadows should end
>>> up in the duplicates list anyway. So the patch shouldn't have any
>>> effect.
>>
>> Okay, so the reason for the remaining crash is still unclear at least
>> for me.
>
> Yeah, that's a really good question. Can you share the call stack of the
> problem once more?

Attaching the dmesg again.

amdgpu_gtt_mgr_alloc+0x23 resolves to the check

    if (node->start != AMDGPU_BO_INVALID_OFFSET)

amdgpu_vm_update_page_directory+0x23f is

	r = amdgpu_ttm_bind(&pt_shadow->tbo,
			    &pt_shadow->tbo.mem);

Nicolai
[  545.477646] BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
[  545.477689] IP: [<ffffffffc0533ca3>] amdgpu_gtt_mgr_alloc+0x23/0x150 [amdgpu]
[  545.477764] PGD 7e384a067
[  545.477775] PUD 7f4a84067
[  545.477786] PMD 0

[  545.477797] Oops: 0000 [#1] SMP
[  545.477810] Modules linked in: binfmt_misc nls_iso8859_1 eeepc_wmi asus_wmi video sparse_keymap mxm_wmi joydev input_leds edac_mce_amd edac_core kvm_amd kvm irqbypass snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi snd_hda_intel serio_raw snd_hda_codec snd_hda_core snd_hwdep fam15h_power k10temp snd_pcm snd_seq_midi i2c_piix4 snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device snd_timer tpm_infineon snd soundcore wmi mac_hid shpchp parport_pc ppdev lp parport autofs4 algif_skcipher af_alg hid_generic usbhid hid dm_crypt amdkfd amd_iommu_v2 amdgpu crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel i2c_algo_bit aes_x86_64 drm_kms_helper glue_helper lrw syscopyarea gf128mul sysfillrect ablk_helper sysimgblt cryptd fb_sys_fops ttm psmouse drm ahci r8169 libahci mii fjes
[  545.478165] CPU: 5 PID: 29619 Comm: glcts Not tainted 4.9.0-rc6-tip+drm-next-2 #104
[  545.478191] Hardware name: To be filled by O.E.M. To be filled by O.E.M./M5A97 LE R2.0, BIOS 2601 03/24/2015
[  545.478225] task: ffff8be896f4d580 task.stack: ffffb7af4c3f4000
[  545.478246] RIP: 0010:[<ffffffffc0533ca3>]  [<ffffffffc0533ca3>] amdgpu_gtt_mgr_alloc+0x23/0x150 [amdgpu]
[  545.478301] RSP: 0018:ffffb7af4c3f7a28  EFLAGS: 00010296
[  545.478320] RAX: 7fffffffffffffff RBX: ffff8be8967e6180 RCX: ffff8be82806ec90
[  545.478343] RDX: 0000000000000000 RSI: ffff8be82806ec58 RDI: ffff8be8957c9980
[  545.478367] RBP: ffffb7af4c3f7a88 R08: ffff8be8bed5c540 R09: ffff8be89e003900
[  545.478390] R10: ffff8be896af4cc0 R11: ffff8be8957c1900 R12: 0000000000000000
[  545.478412] R13: 0000000000000000 R14: ffff8be8967e6228 R15: ffff8be82806fc00
[  545.478437] FS:  00007ff4415f2740(0000) GS:ffff8be8bed40000(0000) knlGS:0000000000000000
[  545.478462] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  545.478481] CR2: 0000000000000048 CR3: 00000007c031a000 CR4: 00000000000406e0
[  545.478506] Stack:
[  545.478513]  0000000000000000 00000000d3451c09 ffff8be78308a9d8 0000000000000000
[  545.478544]  ffff8be8957c8000 0000000000001a00 ffff8be863b86000 ffff8be8967e6180
[  545.478575]  ffff8be82806ec90 0000000000000000 ffff8be8967e6228 ffff8be82806fc00
[  545.478604] Call Trace:
[  545.478632]  [<ffffffffc0516bf1>] amdgpu_ttm_bind+0x61/0x160 [amdgpu]
[  545.478672]  [<ffffffffc052f58f>] amdgpu_vm_update_page_directory+0x23f/0x4c0 [amdgpu]
[  545.478717]  [<ffffffffc052124a>] amdgpu_cs_ioctl+0xd8a/0x1400 [amdgpu]
[  545.478759]  [<ffffffffc02f9e76>] drm_ioctl+0x1f6/0x4a0 [drm]
[  545.478794]  [<ffffffffc05204c0>] ? amdgpu_cs_find_mapping+0xa0/0xa0 [amdgpu]
[  545.478823]  [<ffffffff8b0b8255>] ? update_load_avg+0x75/0x390
[  545.478858]  [<ffffffffc050404c>] amdgpu_drm_ioctl+0x4c/0x80 [amdgpu]
[  545.478882]  [<ffffffff8b241e81>] do_vfs_ioctl+0xa1/0x5d0
[  545.478902]  [<ffffffff8b842e2a>] ? __schedule+0x23a/0x6f0
[  545.478923]  [<ffffffff8b242429>] SyS_ioctl+0x79/0x90
[  545.478942]  [<ffffffff8b848bfb>] entry_SYSCALL_64_fastpath+0x1e/0xad
[  545.478965] Code: 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 b8 ff ff ff ff ff ff ff 7f 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 38 4c 8b 21 <49> 39 44 24 48 74 11 31 c0 48 83 c4 38 5b 41 5c 41 5d 41 5e 41
[  545.479133] RIP  [<ffffffffc0533ca3>] amdgpu_gtt_mgr_alloc+0x23/0x150 [amdgpu]
[  545.479179]  RSP <ffffb7af4c3f7a28>
[  545.479192] CR2: 0000000000000048
[  545.485015] ---[ end trace 390c3d6250a76506 ]---
On 13.12.2016 10:48, Christian König wrote:
>>>> The attached patch has fixed these crashes for me so far, but it's
>>>> very heavy-handed: it collects all page table shadows and the page
>>>> directory shadow and adds them all to the reservations for the callers
>>>> of amdgpu_vm_update_page_directory.
>>>
>>> That is most likely just a timing change, cause the shadows should end
>>> up in the duplicates list anyway. So the patch shouldn't have any
>>> effect.
>>
>> Okay, so the reason for the remaining crash is still unclear at least
>> for me.
>
> Yeah, that's a really good question. Can you share the call stack of the
> problem once more?

Pretty sure I found the root cause now. amdgpu_vm_validate_pt_bos relies 
on the eviction counter to be able to skip the validation of the page 
tables.

However, moving the shadow page tables out from mem_type TT to SYSTEM 
doesn't count as an eviction (it just unbinds the mapping in the GTT).

Clearly, that's a problem.

The quick fix is to skip the num_evictions check in 
amdgpu_vm_validate_pt_bos. That has worked for me so far.

The next best thing is to add an unbind counter in addition to the 
eviction counter that gets incremented whenever a BO is unbound (so it 
counts a superset of what the eviction counter counts), and then check 
that instead of the eviction counter.

Cheers,
Nicolai
Am 14.12.2016 um 15:22 schrieb Nicolai Hähnle:
> On 13.12.2016 10:48, Christian König wrote:
>>>>> The attached patch has fixed these crashes for me so far, but it's
>>>>> very heavy-handed: it collects all page table shadows and the page
>>>>> directory shadow and adds them all to the reservations for the 
>>>>> callers
>>>>> of amdgpu_vm_update_page_directory.
>>>>
>>>> That is most likely just a timing change, cause the shadows should end
>>>> up in the duplicates list anyway. So the patch shouldn't have any
>>>> effect.
>>>
>>> Okay, so the reason for the remaining crash is still unclear at least
>>> for me.
>>
>> Yeah, that's a really good question. Can you share the call stack of the
>> problem once more?
>
> Pretty sure I found the root cause now. amdgpu_vm_validate_pt_bos 
> relies on the eviction counter to be able to skip the validation of 
> the page tables.
>
> However, moving the shadow page tables out from mem_type TT to SYSTEM 
> doesn't count as an eviction (it just unbinds the mapping in the GTT).
>
> Clearly, that's a problem.

Nice catch!

> The quick fix is to skip the num_evictions check in 
> amdgpu_vm_validate_pt_bos. That has worked for me so far.
>
> The next best thing is to add an unbind counter in addition to the 
> eviction counter that gets incremented whenever a BO is unbound (so it 
> counts a superset of what the eviction counter counts), and then check 
> that instead of the eviction counter.

Well to complicated, we should just make the eviction counter handle 
both events.

That's also the original meaning of it, e.g. unbinding pages from the 
GART is some sort of eviction as well in this case.

Regards,
Christian.

>
> Cheers,
> Nicolai
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 14.12.2016 15:56, Christian König wrote:
> Am 14.12.2016 um 15:22 schrieb Nicolai Hähnle:
>> On 13.12.2016 10:48, Christian König wrote:
>>>>>> The attached patch has fixed these crashes for me so far, but it's
>>>>>> very heavy-handed: it collects all page table shadows and the page
>>>>>> directory shadow and adds them all to the reservations for the
>>>>>> callers
>>>>>> of amdgpu_vm_update_page_directory.
>>>>>
>>>>> That is most likely just a timing change, cause the shadows should end
>>>>> up in the duplicates list anyway. So the patch shouldn't have any
>>>>> effect.
>>>>
>>>> Okay, so the reason for the remaining crash is still unclear at least
>>>> for me.
>>>
>>> Yeah, that's a really good question. Can you share the call stack of the
>>> problem once more?
>>
>> Pretty sure I found the root cause now. amdgpu_vm_validate_pt_bos
>> relies on the eviction counter to be able to skip the validation of
>> the page tables.
>>
>> However, moving the shadow page tables out from mem_type TT to SYSTEM
>> doesn't count as an eviction (it just unbinds the mapping in the GTT).
>>
>> Clearly, that's a problem.
>
> Nice catch!
>
>> The quick fix is to skip the num_evictions check in
>> amdgpu_vm_validate_pt_bos. That has worked for me so far.
>>
>> The next best thing is to add an unbind counter in addition to the
>> eviction counter that gets incremented whenever a BO is unbound (so it
>> counts a superset of what the eviction counter counts), and then check
>> that instead of the eviction counter.
>
> Well to complicated, we should just make the eviction counter handle
> both events.
>
> That's also the original meaning of it, e.g. unbinding pages from the
> GART is some sort of eviction as well in this case.

I'll do that then. The counter does get exposed to user-space, 
implementing the GL_NVX_gpu_memory_info extension, so it's a change of 
behavior there. But that extension is pretty under-specified anyway, and 
it does make sense to count getting kicked out of GART as an eviction.

Cheers,
Nicolai


>
> Regards,
> Christian.
>
>>
>> Cheers,
>> Nicolai
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>