Nouveau - Aug 2019 - [PATCH] nouveau/hmm: map pages after migration

On 8/8/19 12:07 AM, Christoph Hellwig wrote:
> On Wed, Aug 07, 2019 at 08:02:14AM -0700, Ralph Campbell wrote:
>> When memory is migrated to the GPU it is likely to be accessed by GPU
>> code soon afterwards. Instead of waiting for a GPU fault, map the
>> migrated memory into the GPU page tables with the same access
permissions
>> as the source CPU page table entries. This preserves copy on write
>> semantics.
>>
>> Signed-off-by: Ralph Campbell <rcampbell at nvidia.com>
>> Cc: Christoph Hellwig <hch at lst.de>
>> Cc: Jason Gunthorpe <jgg at mellanox.com>
>> Cc: "Jérôme Glisse" <jglisse at redhat.com>
>> Cc: Ben Skeggs <bskeggs at redhat.com>
>> ---
>>
>> This patch is based on top of Christoph Hellwig's 9 patch series
>> https://lore.kernel.org/linux-mm/20190729234611.GC7171 at
redhat.com/T/#u
>> "turn the hmm migrate_vma upside down" but without patch 9
>> "mm: remove the unused MIGRATE_PFN_WRITE" and adds a use for
the flag.
> 
> This looks useful.  I've already dropped that patch for the pending
> resend.
Thanks.
> 
>>   static unsigned long nouveau_dmem_migrate_copy_one(struct nouveau_drm
*drm,
>> -		struct vm_area_struct *vma, unsigned long addr,
>> -		unsigned long src, dma_addr_t *dma_addr)
>> +		struct vm_area_struct *vma, unsigned long src,
>> +		dma_addr_t *dma_addr, u64 *pfn)
> 
> I'll pick up the removal of the not needed addr argument for the patch
> introducing nouveau_dmem_migrate_copy_one, thanks,
> 
>>   static void nouveau_dmem_migrate_chunk(struct migrate_vma *args,
>> -		struct nouveau_drm *drm, dma_addr_t *dma_addrs)
>> +		struct nouveau_drm *drm, dma_addr_t *dma_addrs, u64 *pfns)
>>   {
>>   	struct nouveau_fence *fence;
>>   	unsigned long addr = args->start, nr_dma = 0, i;
>>   
>>   	for (i = 0; addr < args->end; i++) {
>>   		args->dst[i] = nouveau_dmem_migrate_copy_one(drm, args->vma,
>> -				addr, args->src[i], &dma_addrs[nr_dma]);
>> +				args->src[i], &dma_addrs[nr_dma], &pfns[i]);
> 
> Nit: I find the &pfns[i] way to pass the argument a little weird to
read.
> Why not "pfns + i"?
OK, will do in v2.
Should I convert to "dma_addrs + nr_dma" too?
>> +u64 *
>> +nouveau_pfns_alloc(unsigned long npages)
>> +{
>> +	struct nouveau_pfnmap_args *args;
>> +
>> +	args = kzalloc(sizeof(*args) + npages * sizeof(args->p.phys[0]),
> 
> Can we use struct_size here?
Yes, good suggestion.
> 
>> +	int ret;
>> +
>> +	if (!svm)
>> +		return;
>> +
>> +	mutex_lock(&svm->mutex);
>> +	svmm = nouveau_find_svmm(svm, mm);
>> +	if (!svmm) {
>> +		mutex_unlock(&svm->mutex);
>> +		return;
>> +	}
>> +	mutex_unlock(&svm->mutex);
> 
> Given that nouveau_find_svmm doesn't take any kind of reference, what
> gurantees svmm doesn't go away after dropping the lock?
I asked Ben and Jerome about this too.
I'm still looking into it.
> 
>> @@ -44,5 +49,19 @@ static inline int nouveau_svmm_bind(struct
drm_device *device, void *p,
>>   {
>>   	return -ENOSYS;
>>   }
>> +
>> +u64 *nouveau_pfns_alloc(unsigned long npages)
>> +{
>> +	return NULL;
>> +}
>> +
>> +void nouveau_pfns_free(u64 *pfns)
>> +{
>> +}
>> +
>> +void nouveau_pfns_map(struct nouveau_drm *drm, struct mm_struct *mm,
>> +		      unsigned long addr, u64 *pfns, unsigned long npages)
>> +{
>> +}
>>   #endif /* IS_ENABLED(CONFIG_DRM_NOUVEAU_SVM) */
> 
> nouveau_dmem.c and nouveau_svm.c are both built conditional on
> CONFIG_DRM_NOUVEAU_SVM, so there is no need for stubs here.
> 
Good point. I'll remove them in v2.

On Thu, Aug 08, 2019 at 02:29:34PM -0700, Ralph Campbell
wrote:
>>>   {
>>>   	struct nouveau_fence *fence;
>>>   	unsigned long addr = args->start, nr_dma = 0, i;
>>>     	for (i = 0; addr < args->end; i++) {
>>>   		args->dst[i] = nouveau_dmem_migrate_copy_one(drm,
args->vma,
>>> -				addr, args->src[i], &dma_addrs[nr_dma]);
>>> +				args->src[i], &dma_addrs[nr_dma], &pfns[i]);
>>
>> Nit: I find the &pfns[i] way to pass the argument a little weird to
read.
>> Why not "pfns + i"?
>
> OK, will do in v2.
> Should I convert to "dma_addrs + nr_dma" too?
I'll fix it up for v3 of the migrate_vma series.  This is a leftover
from passing an args structure.

On something vaguely related to this patch:

You use the NVIF_VMM_PFNMAP_V0_V* defines from nvif/if000c.h, which are
a little odd as we only ever set these bits, but they also don't seem
to appear to be in values that are directly fed to the hardware.

On the other hand mmu/vmm.h defines a set of NVIF_VMM_PFNMAP_V0_*
constants with similar names and identical values, and those are used
in mmu/vmmgp100.c and what appears to finally do the low-level dma
mapping and talking to the hardware.  Are these two sets of constants
supposed to be the same?  Are the actual hardware values or just a
driver internal interface?

On 8/10/19 4:13 AM, Christoph Hellwig wrote:
> On something vaguely related to this patch:
> 
> You use the NVIF_VMM_PFNMAP_V0_V* defines from nvif/if000c.h, which are
> a little odd as we only ever set these bits, but they also don't seem
> to appear to be in values that are directly fed to the hardware.
> 
> On the other hand mmu/vmm.h defines a set of NVIF_VMM_PFNMAP_V0_*
Yes, I see NVKM_VMM_PFN_*
> constants with similar names and identical values, and those are used
> in mmu/vmmgp100.c and what appears to finally do the low-level dma
> mapping and talking to the hardware.  Are these two sets of constants
> supposed to be the same?  Are the actual hardware values or just a
> driver internal interface?
It looks a bit odd to me too.
I don't really know the structure/history of nouveau.
Perhaps Ben Skeggs can shed more light on your question.