Re: [PATCH 4/5] nouveau: unlock mmap_sem on all errors from nouveau_range_fault

[Christoph Hellwig]

On Wed, Jul 03, 2019 at 01:46:02PM -0700, Ralph Campbell wrote:
> You can delete the comment "With the old API the driver must ..."
> (not visible in the patch here).

Sure.

> I suggest moving the two assignments:
> 	range->default_flags = 0;
> 	range->pfn_flags_mask = -1UL;
> to just above the "again:" where the other range.xxx fields are
> initialized in nouveau_svm_fault().

For now I really just want to move the code around.  As Jason pointed
out the flow will need some major rework, and I'd rather not mess
with little things like this for now.  Especially as I assume Jerome
must have an update to the proper API ready given that he both
wrote that new API and the nouveau code.

> You can delete this comment (only the first line is visible here)
> since it is about the "old API".

Ok.

> Also, it should return -EBUSY not -EAGAIN since it means there was a
> range invalidation collision (similar to hmm_range_fault() if
> !range->valid).

Yes, probably.


>> @@ -515,15 +517,14 @@ nouveau_range_fault(struct hmm_mirror *mirror, struct hmm_range *range,
>>     	ret = hmm_range_fault(range, block);
>
> nouveau_range_fault() is only called with "block = true" so
> could eliminate the block parameter and pass true here.

Indeed.

>
>>   	if (ret <= 0) {
>> -		if (ret == -EBUSY || !ret) {
>> -			/* Same as above, drop mmap_sem to match old API. */
>> -			up_read(&range->vma->vm_mm->mmap_sem);
>> -			ret = -EBUSY;
>> -		} else if (ret == -EAGAIN)
>> +		if (ret == 0)
>>   			ret = -EBUSY;
>> +		if (ret != -EAGAIN)
>> +			up_read(&range->vma->vm_mm->mmap_sem);
>
> Can ret == -EAGAIN happen if "block = true"?

I don't think so, we can remove that.

> Generally, I prefer the read_down()/read_up() in the same function
> (i.e., nouveau_svm_fault()) but I can see why it should be here
> if hmm_range_fault() can return with mmap_sem unlocked.

Yes, in the long run this all needs a major cleanup..


>>   @@ -718,8 +719,8 @@ nouveau_svm_fault(struct nvif_notify *notify)
>>   						NULL);
>>   			svmm->vmm->vmm.object.client->super = false;
>>   			mutex_unlock(&svmm->mutex);
>> +			up_read(&svmm->mm->mmap_sem);
>>   		}
>> -		up_read(&svmm->mm->mmap_sem);
>>   
>
> The "else" case should check for -EBUSY and goto again.

It should if I were trying to fix this.  But this is just code
inspection and I don't even have the hardware, so I'll have to leave
that for someone who can do real development on the driver.