Re: [RESEND PATCH 2/3] nouveau: fix mixed normal and device private page migration

[Ralph Campbell]

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.