[PATCH] nouveau: fix the start/end range for migration

[PATCH] nouveau: fix the start/end range for migration

[Ralph Campbell]

The user level OpenCL code shouldn't have to align start and end
addresses to a page boundary. That is better handled in the nouveau
driver. The npages field is also redundant since it can be computed
from the start and end addresses.

Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
---

This is for Ben Skegg's nouveau tree.

I have been working with Karol Herbst on the OpenCL mesa changes for
nouveau which will be merged upstream soon.
With or without those changes, the user visible effect of this patch
only extends the range by one page (round up vs. round down to page
boundary).

 drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 2df1c0460559..888aa0908c5a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 	struct nouveau_cli *cli = nouveau_cli(file_priv);
 	struct drm_nouveau_svm_bind *args = data;
 	unsigned target, cmd, priority;
-	unsigned long addr, end, size;
+	unsigned long addr, end;
 	struct mm_struct *mm;
 
 	args->va_start &= PAGE_MASK;
-	args->va_end &= PAGE_MASK;
+	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
+	/* If no end address is given, assume a single page. */
+	if (args->va_end == 0)
+		args->va_end = args->va_start + PAGE_SIZE;
 
 	/* Sanity check arguments */
 	if (args->reserved0 || args->reserved1)
@@ -118,8 +121,6 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 		return -EINVAL;
 	if (args->va_start >= args->va_end)
 		return -EINVAL;
-	if (!args->npages)
-		return -EINVAL;
 
 	cmd = args->header >> NOUVEAU_SVM_BIND_COMMAND_SHIFT;
 	cmd &= NOUVEAU_SVM_BIND_COMMAND_MASK;
@@ -151,12 +152,6 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 	if (args->stride)
 		return -EINVAL;
 
-	size = ((unsigned long)args->npages) << PAGE_SHIFT;
-	if ((args->va_start + size) <= args->va_start)
-		return -EINVAL;
-	if ((args->va_start + size) > args->va_end)
-		return -EINVAL;
-
 	/*
 	 * Ok we are ask to do something sane, for now we only support migrate
 	 * commands but we will add things like memory policy (what to do on
@@ -171,7 +166,7 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	for (addr = args->va_start, end = args->va_start + size; addr < end;) {
+	for (addr = args->va_start, end = args->va_end; addr < end;) {
 		struct vm_area_struct *vma;
 		unsigned long next;
 
-- 
2.20.1

Re: [PATCH] nouveau: fix the start/end range for migration

[Jason Gunthorpe]

On Thu, Aug 27, 2020 at 02:37:44PM -0700, Ralph Campbell wrote:
> The user level OpenCL code shouldn't have to align start and end
> addresses to a page boundary. That is better handled in the nouveau
> driver. The npages field is also redundant since it can be computed
> from the start and end addresses.
> 
> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
> 
> This is for Ben Skegg's nouveau tree.
> 
> I have been working with Karol Herbst on the OpenCL mesa changes for
> nouveau which will be merged upstream soon.
> With or without those changes, the user visible effect of this patch
> only extends the range by one page (round up vs. round down to page
> boundary).
> 
>  drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 2df1c0460559..888aa0908c5a 100644
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
>  	struct nouveau_cli *cli = nouveau_cli(file_priv);
>  	struct drm_nouveau_svm_bind *args = data;
>  	unsigned target, cmd, priority;
> -	unsigned long addr, end, size;
> +	unsigned long addr, end;
>  	struct mm_struct *mm;
>  
>  	args->va_start &= PAGE_MASK;
> -	args->va_end &= PAGE_MASK;
> +	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
> +	/* If no end address is given, assume a single page. */
> +	if (args->va_end == 0)
> +		args->va_end = args->va_start + PAGE_SIZE;

That is really weird, how is it useful for the kernel to map a region
of unknown size and alignment to the GPU?

Jason

Re: [PATCH] nouveau: fix the start/end range for migration

[Ralph Campbell]

On 8/31/20 4:51 AM, Jason Gunthorpe wrote:
> On Thu, Aug 27, 2020 at 02:37:44PM -0700, Ralph Campbell wrote:
>> The user level OpenCL code shouldn't have to align start and end
>> addresses to a page boundary. That is better handled in the nouveau
>> driver. The npages field is also redundant since it can be computed
>> from the start and end addresses.
>>
>> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
>>
>> This is for Ben Skegg's nouveau tree.
>>
>> I have been working with Karol Herbst on the OpenCL mesa changes for
>> nouveau which will be merged upstream soon.
>> With or without those changes, the user visible effect of this patch
>> only extends the range by one page (round up vs. round down to page
>> boundary).
>>
>>   drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
>>   1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> index 2df1c0460559..888aa0908c5a 100644
>> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> @@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
>>   	struct nouveau_cli *cli = nouveau_cli(file_priv);
>>   	struct drm_nouveau_svm_bind *args = data;
>>   	unsigned target, cmd, priority;
>> -	unsigned long addr, end, size;
>> +	unsigned long addr, end;
>>   	struct mm_struct *mm;
>>   
>>   	args->va_start &= PAGE_MASK;
>> -	args->va_end &= PAGE_MASK;
>> +	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
>> +	/* If no end address is given, assume a single page. */
>> +	if (args->va_end == 0)
>> +		args->va_end = args->va_start + PAGE_SIZE;
> 
> That is really weird, how is it useful for the kernel to map a region
> of unknown size and alignment to the GPU?
> 
> Jason

I agree it is somewhat weird. The OpenCL 2.2 specification says that
clEnqueueSVMMigrateMem() takes an array of pointers and sizes (in bytes)
but the size is optional. There is no alignment required.
This "works" because the pointers have to be allocated with clSVMAlloc()
and presumably, the implementation for clSVMAlloc()
keeps track of the length allocated and can fill that in if size is zero.
However, requiring all allocations to be made with clSVMAlloc() defeats the
goal of being able to use regular malloc() and mmap() allocations for OpenCL
implementations that support fine-grained system allocations.
(See https://github.com/KhronosGroup/OpenCL-Docs/issues/392)

So if the size isn't specified, the most logical choices are do nothing and
return OK, return an error, or assume that at least one byte is being migrated
and try migrate it.

Re: [PATCH] nouveau: fix the start/end range for migration

[Jason Gunthorpe]

On Mon, Aug 31, 2020 at 10:21:41AM -0700, Ralph Campbell wrote:
> 
> On 8/31/20 4:51 AM, Jason Gunthorpe wrote:
> > On Thu, Aug 27, 2020 at 02:37:44PM -0700, Ralph Campbell wrote:
> > > The user level OpenCL code shouldn't have to align start and end
> > > addresses to a page boundary. That is better handled in the nouveau
> > > driver. The npages field is also redundant since it can be computed
> > > from the start and end addresses.
> > > 
> > > Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
> > > 
> > > This is for Ben Skegg's nouveau tree.
> > > 
> > > I have been working with Karol Herbst on the OpenCL mesa changes for
> > > nouveau which will be merged upstream soon.
> > > With or without those changes, the user visible effect of this patch
> > > only extends the range by one page (round up vs. round down to page
> > > boundary).
> > > 
> > >   drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
> > >   1 file changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> > > index 2df1c0460559..888aa0908c5a 100644
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> > > @@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
> > >   	struct nouveau_cli *cli = nouveau_cli(file_priv);
> > >   	struct drm_nouveau_svm_bind *args = data;
> > >   	unsigned target, cmd, priority;
> > > -	unsigned long addr, end, size;
> > > +	unsigned long addr, end;
> > >   	struct mm_struct *mm;
> > >   	args->va_start &= PAGE_MASK;
> > > -	args->va_end &= PAGE_MASK;
> > > +	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
> > > +	/* If no end address is given, assume a single page. */
> > > +	if (args->va_end == 0)
> > > +		args->va_end = args->va_start + PAGE_SIZE;
> > 
> > That is really weird, how is it useful for the kernel to map a region
> > of unknown size and alignment to the GPU?
> > 
> > Jason
> 
> I agree it is somewhat weird. The OpenCL 2.2 specification says that
> clEnqueueSVMMigrateMem() takes an array of pointers and sizes (in bytes)
> but the size is optional. There is no alignment required.
> This "works" because the pointers have to be allocated with clSVMAlloc()
> and presumably, the implementation for clSVMAlloc()
> keeps track of the length allocated and can fill that in if size is zero.
> However, requiring all allocations to be made with clSVMAlloc() defeats the
> goal of being able to use regular malloc() and mmap() allocations for OpenCL
> implementations that support fine-grained system allocations.
> (See https://github.com/KhronosGroup/OpenCL-Docs/issues/392)
> 
> So if the size isn't specified, the most logical choices are do nothing and
> return OK, return an error, or assume that at least one byte is being migrated
> and try migrate it.

So if the app migrates the wrong memory then nothing bad happens, it
just might not get the performance from migration? Seems find but
really weird.

Jason

Re: [PATCH] nouveau: fix the start/end range for migration

[Ralph Campbell]

On 8/31/20 11:02 AM, Jason Gunthorpe wrote:
> On Mon, Aug 31, 2020 at 10:21:41AM -0700, Ralph Campbell wrote:
>>
>> On 8/31/20 4:51 AM, Jason Gunthorpe wrote:
>>> On Thu, Aug 27, 2020 at 02:37:44PM -0700, Ralph Campbell wrote:
>>>> The user level OpenCL code shouldn't have to align start and end
>>>> addresses to a page boundary. That is better handled in the nouveau
>>>> driver. The npages field is also redundant since it can be computed
>>>> from the start and end addresses.
>>>>
>>>> Signed-off-by: Ralph Campbell <rcampbell@nvidia.com>
>>>>
>>>> This is for Ben Skegg's nouveau tree.
>>>>
>>>> I have been working with Karol Herbst on the OpenCL mesa changes for
>>>> nouveau which will be merged upstream soon.
>>>> With or without those changes, the user visible effect of this patch
>>>> only extends the range by one page (round up vs. round down to page
>>>> boundary).
>>>>
>>>>    drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
>>>>    1 file changed, 6 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
>>>> index 2df1c0460559..888aa0908c5a 100644
>>>> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
>>>> @@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
>>>>    	struct nouveau_cli *cli = nouveau_cli(file_priv);
>>>>    	struct drm_nouveau_svm_bind *args = data;
>>>>    	unsigned target, cmd, priority;
>>>> -	unsigned long addr, end, size;
>>>> +	unsigned long addr, end;
>>>>    	struct mm_struct *mm;
>>>>    	args->va_start &= PAGE_MASK;
>>>> -	args->va_end &= PAGE_MASK;
>>>> +	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
>>>> +	/* If no end address is given, assume a single page. */
>>>> +	if (args->va_end == 0)
>>>> +		args->va_end = args->va_start + PAGE_SIZE;
>>>
>>> That is really weird, how is it useful for the kernel to map a region
>>> of unknown size and alignment to the GPU?
>>>
>>> Jason
>>
>> I agree it is somewhat weird. The OpenCL 2.2 specification says that
>> clEnqueueSVMMigrateMem() takes an array of pointers and sizes (in bytes)
>> but the size is optional. There is no alignment required.
>> This "works" because the pointers have to be allocated with clSVMAlloc()
>> and presumably, the implementation for clSVMAlloc()
>> keeps track of the length allocated and can fill that in if size is zero.
>> However, requiring all allocations to be made with clSVMAlloc() defeats the
>> goal of being able to use regular malloc() and mmap() allocations for OpenCL
>> implementations that support fine-grained system allocations.
>> (See https://github.com/KhronosGroup/OpenCL-Docs/issues/392)
>>
>> So if the size isn't specified, the most logical choices are do nothing and
>> return OK, return an error, or assume that at least one byte is being migrated
>> and try migrate it.
> 
> So if the app migrates the wrong memory then nothing bad happens, it
> just might not get the performance from migration? Seems find but
> really weird.
> 
> Jason
> 

Given the principal of least surprise, I'm thinking the better choice is to
return an error from the driver and leave any other actions to the user level
library. I'll post a v2.