Re: [PATCH] drm/amdkfd: correct the SVM DMA device unmap direction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Am 08.11.24 um 10:15 schrieb Liang, Prike:

[AMD Official Use Only - AMD Internal Distribution Only]


From: Koenig, Christian <Christian.Koenig@xxxxxxx>
Sent: Wednesday, November 6, 2024 8:24 PM
To: Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Liang, Prike
<Prike.Liang@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Kasiviswanathan, Harish
<Harish.Kasiviswanathan@xxxxxxx>
Subject: Re: [PATCH] drm/amdkfd: correct the SVM DMA device unmap direction

Am 05.11.24 um 17:34 schrieb Felix Kuehling:

On 2024-11-05 06:04, Christian König wrote:

Am 05.11.24 um 03:33 schrieb Prike Liang:

The SVM DMA device unmap direction should be same as the DMA map
process.

At least of hand that looks like it's only papering over a major
problem.

Why are DMA ranges for SVM mapped with a direction in the first
place? That is usually not something we should do.

These are DMA mappings of system memory pages. I guess we're creating
DMA mappings only for the access required for the migration, which is
not bidirectional. I see we do something similar for userptr mappings
depending on whether the GPU mapping is read-only or read-write. Is
that wrong for userptrs as well?

I think so, yes. The DMA directions are there to make explicit CPU cache
management and bounce buffers possible.

Since we shouldn't need or even want either for a cache coherent PCIe device we
should probably always use BIDIRECTIONAL.


The DMA core will check the direction when the driver performs DMA unmap, and if the DMA unmap direction does not match the map direction setting, it will report a warning like the following.
Yeah, that is perfectly expected. Doing the unmap with a different setting than the map is clearly a bug. 

The question is rather should the map or the unmap operation be changed?
In your patch you propose to change the unmap operation, but I think that is wrong and the map operation should use BIDIRECTIONAL in the first place. 

Regards,
Christian.


  Meanwhile, for stream DMA unmappings without the DMA_ATTR_SKIP_CPU_SYNC attribute setting, there will be a different cache policy for each DMA direction. So, will this affect the unmap performance when all using the BIDIRECTIONAL setting?

For userptr unmappings, it appears that they are doing the correct thing by using the same direction as the mapping setting.

...... < SNIP>
DMA-API: amdgpu 0000:03:00.0: device driver frees DMA memory with different direction [device address=0x00000001f8263000] [size=4096 bytes] [mapped with DMA_TO_DEVICE] [unmapped with DMA_BIDIRECTIONAL]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360158] WARNING: CPU: 9 PID: 11671 at kernel/dma/debug.c:1028 check_unmap+0x1cc/0x930
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360165] Modules linked in: veth amdgpu(OE) amdxcp drm_exec gpu_sched drm_buddy drm_ttm_helper ttm(OE) drm_suballoc_helper drm_display_helper drm_kms_helper drm i2c_algo_bit video tls rpcsec_gss_krb5 auth_rpcgss nfsv4 nfs lockd grace netfs xt_conntrack xt_MASQUERADE nf_conntrack_netlink xfrm_user xfrm_algo iptable_nat xt_addrtype iptable_filter br_netfilter nvme_fabrics overlay bridge nfnetlink_cttimeout stp llc nfnetlink openvswitch nsh nf_conncount nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c sch_fq_codel intel_rapl_msr amd_atl intel_rapl_common snd_hda_codec_realtek snd_hda_codec_generic snd_hda_scodec_component snd_hda_codec_hdmi sunrpc edac_mce_amd snd_hda_intel snd_intel_dspcfg snd_hda_codec kvm_amd snd_pci_acp6x snd_hda_core snd_acp_config snd_soc_acpi snd_hwdep binfmt_misc kvm snd_pcm nls_iso8859_1 crct10dif_pclmul snd_seq_midi snd_seq_midi_event ghash_clmulni_intel sha512_ssse3 sha256_ssse3 snd_rawmidi sha1_ssse3 aesni_intel crypto_simd cryptd snd_seq input_leds rapl serio_raw wmi_bmof
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360255]  snd_seq_device k10temp snd_timer sp5100_tco snd ccp soundcore ipmi_devintf cm32181 ipmi_msghandler industrialio mac_hid msr parport_pc ppdev lp parport efi_pstore ip_tables x_tables pci_stub crc32_pclmul nvme ahci libahci i2c_piix4 r8169 nvme_core i2c_designware_pci realtek i2c_ccgx_ucsi wmi cdc_ether hid_generic usbnet usbhid r8152 hid mii [last unloaded: drm]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360297] CPU: 9 PID: 11671 Comm: pt_main_thread Tainted: G           OE      6.10.0-custom #492
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360301] Hardware name: AMD Majolica-RN/Majolica-RN, BIOS RMJ1009A 06/13/2021
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360303] RIP: 0010:check_unmap+0x1cc/0x930
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360306] Code: c0 4c 89 4d c8 e8 34 bf 86 00 4c 8b 4d c8 4c 8b 45 c0 48 8b 4d b8 48 89 c6 41 57 4c 89 ea 48 c7 c7 80 49 b4 90 e8 b4 81 f3 ff <0f> 0b 48 c7 c7 04 83 ac 90 e8 76 ba fc ff 41 8b 76 4c 49 8d 7e 50
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360308] RSP: 0018:ffff9c0f855179e0 EFLAGS: 00010086
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360311] RAX: 0000000000000000 RBX: ffffffff9165c138 RCX: 0000000000000027
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360313] RDX: ffff8a6fcf6a1688 RSI: 0000000000000001 RDI: ffff8a6fcf6a1680
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360315] RBP: ffff9c0f85517a30 R08: 00000000000000c9 R09: ffff9c0f85517850
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360316] R10: ffff9c0f85517848 R11: ffffffff90f46328 R12: ffff9c0f85517a40
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360318] R13: ffff8a6c831ec7e0 R14: ffff8a6c819ced80 R15: ffffffff90ac831b
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360320] FS:  00007ff81db1b740(0000) GS:ffff8a6fcf680000(0000) knlGS:0000000000000000
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360322] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360324] CR2: 00007ff310200020 CR3: 0000000158f7a000 CR4: 0000000000350ef0
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360326] Call Trace:
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360327]  <TASK>
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360330]  ? show_regs+0x6d/0x80
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360334]  ? __warn+0x8c/0x140
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360339]  ? check_unmap+0x1cc/0x930
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360343]  ? report_bug+0x193/0x1a0
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360347]  ? handle_bug+0x46/0x80
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360352]  ? exc_invalid_op+0x1d/0x80
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360355]  ? asm_exc_invalid_op+0x1f/0x30
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360362]  ? check_unmap+0x1cc/0x930
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360367]  debug_dma_unmap_page+0x86/0x90
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360373]  ? srso_return_thunk+0x5/0x5f
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360377]  ? rmap_walk+0x28/0x50
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360380]  ? srso_return_thunk+0x5/0x5f
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360383]  ? remove_migration_ptes+0x79/0x80
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360388]  ? srso_return_thunk+0x5/0x5f
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360391]  dma_unmap_page_attrs+0xfa/0x1d0
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360396]  svm_range_dma_unmap_dev+0x8a/0xf0 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360638]  svm_migrate_ram_to_vram+0x361/0x740 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.360840]  svm_migrate_to_vram+0xa8/0xe0 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.361034]  svm_range_set_attr+0xff2/0x1450 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.361232]  svm_ioctl+0x4a/0x50 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.361424]  kfd_ioctl_svm+0x54/0x90 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.361613]  kfd_ioctl+0x3c2/0x530 [amdgpu]
Nov  4 15:45:32 prike-queue-reset kernel: [352033.361798]  ? __pfx_kfd_ioctl_svm+0x10/0x10 [amdgpu

Regards,
     Prike


Regards,
Christian.


Regards,
   Felix



Regards,
Christian.


Signed-off-by: Prike Liang <Prike.Liang@xxxxxxx>
---
   drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 4 ++--
   drivers/gpu/drm/amd/amdkfd/kfd_svm.c     | 6 +++---
   drivers/gpu/drm/amd/amdkfd/kfd_svm.h     | 3 ++-
   3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
index eacfeb32f35d..9d83bb9dd004 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
@@ -445,7 +445,7 @@ svm_migrate_vma_to_vram(struct kfd_node *node,
struct svm_range *prange,
       pr_debug("successful/cpages/npages 0x%lx/0x%lx/0x%lx\n",
                mpages, cpages, migrate.npages);
   -    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages);
+    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages,
DMA_TO_DEVICE);
     out_free:
       kvfree(buf);
@@ -750,7 +750,7 @@ svm_migrate_vma_to_ram(struct kfd_node *node,
struct svm_range *prange,
       svm_migrate_copy_done(adev, mfence);
       migrate_vma_finalize(&migrate);
   -    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages);
+    svm_range_dma_unmap_dev(adev->dev, scratch, 0, npages,
DMA_FROM_DEVICE);
     out_free:
       kvfree(buf);
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 3e2911895c74..c21485fe6cbb 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -233,9 +233,9 @@ svm_range_dma_map(struct svm_range *prange,
unsigned long *bitmap,
   }
     void svm_range_dma_unmap_dev(struct device *dev, dma_addr_t
*dma_addr,
-             unsigned long offset, unsigned long npages)
+             unsigned long offset, unsigned long npages,
+                enum dma_data_direction dir)
   {
-    enum dma_data_direction dir = DMA_BIDIRECTIONAL;
       int i;
         if (!dma_addr)
@@ -272,7 +272,7 @@ void svm_range_dma_unmap(struct svm_range
*prange)
           }
           dev = &pdd->dev->adev->pdev->dev;
   -        svm_range_dma_unmap_dev(dev, dma_addr, 0,
prange->npages);
+        svm_range_dma_unmap_dev(dev, dma_addr, 0, prange->npages,
DMA_BIDIRECTIONAL);
       }
   }
   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
index bddd24f04669..5370d68bc5b2 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.h
@@ -182,7 +182,8 @@ void svm_range_add_list_work(struct
svm_range_list *svms,
                    enum svm_work_list_ops op);
   void schedule_deferred_list_work(struct svm_range_list *svms);
   void svm_range_dma_unmap_dev(struct device *dev, dma_addr_t
*dma_addr,
-             unsigned long offset, unsigned long npages);
+             unsigned long offset, unsigned long npages,
+             enum dma_data_direction dir);
   void svm_range_dma_unmap(struct svm_range *prange);
   int svm_range_get_info(struct kfd_process *p, uint32_t
*num_svm_ranges,
                  uint64_t *svm_priv_data_size);
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux