Christoph Hellwig
2020-Jun-24 07:23 UTC
[Nouveau] [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
On Mon, Jun 22, 2020 at 04:38:53PM -0700, Ralph Campbell wrote:> The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will > migrate memory in the given address range to device private memory. The > source pages might already have been migrated to device private memory. > In that case, the source struct page is not checked to see if it is > a device private page and incorrectly computes the GPU's physical > address of local memory leading to data corruption. > Fix this by checking the source struct page and computing the correct > physical address.I'm really worried about all this delicate code to fix the mixed ranges. Can't we make it clear at the migrate_vma_* level if we want to migrate from or two device private memory, and then skip all the work for regions of memory that already are in the right place? This might be a little more work initially, but I think it leads to a much better API.
Ralph Campbell
2020-Jun-24 18:10 UTC
[Nouveau] [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
On 6/24/20 12:23 AM, Christoph Hellwig wrote:> On Mon, Jun 22, 2020 at 04:38:53PM -0700, Ralph Campbell wrote: >> The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will >> migrate memory in the given address range to device private memory. The >> source pages might already have been migrated to device private memory. >> In that case, the source struct page is not checked to see if it is >> a device private page and incorrectly computes the GPU's physical >> address of local memory leading to data corruption. >> Fix this by checking the source struct page and computing the correct >> physical address. > > I'm really worried about all this delicate code to fix the mixed > ranges. Can't we make it clear at the migrate_vma_* level if we want > to migrate from or two device private memory, and then skip all the work > for regions of memory that already are in the right place? This might be > a little more work initially, but I think it leads to a much better > API. >The current code does encode the direction with src_owner != NULL meaning device private to system memory and src_owner == NULL meaning system memory to device private memory. This patch would obviously defeat that so perhaps a flag could be added to the struct migrate_vma to indicate the direction but I'm unclear how that makes things less delicate. Can you expand on what you are worried about? The issue with invalidations might be better addressed by letting the device driver handle device private page TLB invalidations when migrating to system memory and changing migrate_vma_setup() to only invalidate CPU TLB entries for normal pages being migrated to device private memory. If a page isn't migrating, it seems inefficient to invalidate those TLB entries. Any other suggestions?
Ralph Campbell
2020-Jun-25 17:25 UTC
[Nouveau] [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
Making sure to include linux-mm and Bharata B Rao for IBM's use of migrate_vma*(). On 6/24/20 11:10 AM, Ralph Campbell wrote:> > On 6/24/20 12:23 AM, Christoph Hellwig wrote: >> On Mon, Jun 22, 2020 at 04:38:53PM -0700, Ralph Campbell wrote: >>> The OpenCL function clEnqueueSVMMigrateMem(), without any flags, will >>> migrate memory in the given address range to device private memory. The >>> source pages might already have been migrated to device private memory. >>> In that case, the source struct page is not checked to see if it is >>> a device private page and incorrectly computes the GPU's physical >>> address of local memory leading to data corruption. >>> Fix this by checking the source struct page and computing the correct >>> physical address. >> >> I'm really worried about all this delicate code to fix the mixed >> ranges.? Can't we make it clear at the migrate_vma_* level if we want >> to migrate from or two device private memory, and then skip all the work >> for regions of memory that already are in the right place?? This might be >> a little more work initially, but I think it leads to a much better >> API. >> > > The current code does encode the direction with src_owner != NULL meaning > device private to system memory and src_owner == NULL meaning system > memory to device private memory. This patch would obviously defeat that > so perhaps a flag could be added to the struct migrate_vma to indicate the > direction but I'm unclear how that makes things less delicate. > Can you expand on what you are worried about? > > The issue with invalidations might be better addressed by letting the device > driver handle device private page TLB invalidations when migrating to > system memory and changing migrate_vma_setup() to only invalidate CPU > TLB entries for normal pages being migrated to device private memory. > If a page isn't migrating, it seems inefficient to invalidate those TLB > entries. > > Any other suggestions?After a night's sleep, I think this might work. What do others think? 1) Add a new MMU_NOTIFY_MIGRATE enum to mmu_notifier_event. 2) Change migrate_vma_collect() to use the new MMU_NOTIFY_MIGRATE event type. 3) Modify nouveau_svmm_invalidate_range_start() to simply return (no invalidations) for MMU_NOTIFY_MIGRATE mmu notifier callbacks. 4) Leave the src_owner check in migrate_vma_collect_pmd() for normal pages so if the device driver is migrating normal pages to device private memory, the driver would set src_owner = NULL and already migrated device private pages would be skipped. Since the mmu notifier callback did nothing, the device private entries remain valid in the device's MMU. migrate_vma_collect_pmd() would still invalidate the CPU page tables for migrated normal pages. If the driver is migrating device private pages to system memory, it would set src_owner != NULL, normal pages would be skipped, but now the device driver has to invalidate device MMU mappings in the "alloc and copy" before doing the copy. This would be after migrate_vma_setup() returns so the list of migrating device pages is known to the driver. The rest of the migrate_vma_pages() and migrate_vma_finalize() remain the same.
Reasonably Related Threads
- [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
- [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
- [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
- [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration
- [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration