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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <96fcb138-f258-7d11-7c93-114269f67f3e@nvidia.com>
Date:   Mon, 31 Aug 2020 11:10:53 -0700
From:   Ralph Campbell <rcampbell@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     <nouveau@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        "Jerome Glisse" <jglisse@...hat.com>,
        John Hubbard <jhubbard@...dia.com>,
        "Christoph Hellwig" <hch@....de>, Ben Skeggs <bskeggs@...hat.com>
Subject: Re: [PATCH] nouveau: fix the start/end range for migration


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@...dia.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.