Nouveau - Nov 2019 - [PATCH v2 14/15] drm/amdgpu: Use mmu_range_notifier instead of hmm_mirror

On 2019-11-01 11:12 a.m., Jason Gunthorpe wrote:
> On Fri, Nov 01, 2019 at 02:44:51PM +0000, Yang, Philip wrote:
>>
>>
>> On 2019-10-29 3:25 p.m., Jason Gunthorpe wrote:
>>> On Tue, Oct 29, 2019 at 07:22:37PM +0000, Yang, Philip wrote:
>>>> Hi Jason,
>>>>
>>>> I did quick test after merging amd-staging-drm-next with the
>>>> mmu_notifier branch, which includes this set changes. The test
result
>>>> has different failures, app stuck intermittently, GUI no
display etc. I
>>>> am understanding the changes and will try to figure out the
cause.
>>>
>>> Thanks! I'm not surprised by this given how difficult this
patch was
>>> to make. Let me know if I can assist in any way
>>>
>>> Please ensure to run with lockdep enabled.. Your symptops sounds
sort
>>> of like deadlocking?
>>>
>> Hi Jason,
>>
>> Attached patch fix several issues in amdgpu driver, maybe you can
squash
>> this into patch 14. With this is done, patch 12, 13, 14 is Reviewed-by
>> and Tested-by Philip Yang <philip.yang at amd.com>
> 
> Wow, this is great thanks! Can you clarify what the problems you found
> were? Was the bug the 'return !r' below?
> 
Yes. return !r is critical one, and retry if hmm_range_fault return 
-EBUSY is needed too.
> I'll also add your signed off by
> 
> Here are some remarks:
> 
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>> index cb718a064eb4..c8bbd06f1009 100644
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c
>> @@ -67,21 +67,15 @@ static bool amdgpu_mn_invalidate_gfx(struct
mmu_range_notifier *mrn,
>>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>   	long r;
>>   
>> -	/*
>> -	 * FIXME: Must hold some lock shared with
>> -	 * amdgpu_ttm_tt_get_user_pages_done()
>> -	 */
>> -	mmu_range_set_seq(mrn, cur_seq);
>> +	mutex_lock(&adev->notifier_lock);
>>   
>> -	/* FIXME: Is this necessary? */
>> -	if (!amdgpu_ttm_tt_affect_userptr(bo->tbo.ttm, range->start,
>> -					  range->end))
>> -		return true;
>> +	mmu_range_set_seq(mrn, cur_seq);
>>   
>> -	if (!mmu_notifier_range_blockable(range))
>> +	if (!mmu_notifier_range_blockable(range)) {
>> +		mutex_unlock(&adev->notifier_lock);
>>   		return false;
> 
> This test for range_blockable should be before mutex_lock, I can move
> it up
> 
yes, thanks.
> Also, do you know if notifier_lock is held while calling
> amdgpu_ttm_tt_get_user_pages_done()? Can we add a 'lock assert
held'
> to amdgpu_ttm_tt_get_user_pages_done()?
> 
gpu side hold notifier_lock but kfd side doesn't. kfd side doesn't check
amdgpu_ttm_tt_get_user_pages_done/mmu_range_read_retry return value but 
check mem->invalid flag which is updated from invalidate callback. It 
takes more time to change, I will come to another patch to fix it later.
>> @@ -854,12 +853,20 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo
*bo, struct page **pages)
>>   		r = -EPERM;
>>   		goto out_unlock;
>>   	}
>> +	up_read(&mm->mmap_sem);
>> +	timeout = jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
>> +
>> +retry:
>> +	range->notifier_seq = mmu_range_read_begin(&bo->notifier);
>>   
>> +	down_read(&mm->mmap_sem);
>>   	r = hmm_range_fault(range, 0);
>>   	up_read(&mm->mmap_sem);
>> -
>> -	if (unlikely(r < 0))
>> +	if (unlikely(r <= 0)) {
>> +		if ((r == 0 || r == -EBUSY) && !time_after(jiffies,
timeout))
>> +			goto retry;
>>   		goto out_free_pfns;
>> +	}
> 
> This isn't really right, a retry loop like this needs to go all the
> way to mmu_range_read_retry() and done under the notifier_lock. ie
> mmu_range_read_retry() can fail just as likely as hmm_range_fault()
> can, and drivers are supposed to retry in both cases, with a single
> timeout.
> 
For gpu, check mmu_range_read_retry return value under the notifier_lock 
to do retry is in seperate location, not in same retry loop.
> AFAICT it is a major bug that many places ignore the return code of
> amdgpu_ttm_tt_get_user_pages_done() ???
>
For kfd, explained above.
> However, this is all pre-existing bugs, so I'm OK go ahead with this
> patch as modified. I advise AMD to make a followup patch ..
> 
yes, I will.
> I'll add a FIXME note to this effect.
> 
>>   	for (i = 0; i < ttm->num_pages; i++) {
>>   		pages[i] = hmm_device_entry_to_page(range, range->pfns[i]);
>> @@ -916,7 +923,7 @@ bool amdgpu_ttm_tt_get_user_pages_done(struct
ttm_tt *ttm)
>>   		gtt->range = NULL;
>>   	}
>>   
>> -	return r;
>> +	return !r;
> 
> Ah is this the major error? hmm_range_valid() is inverted vs
> mmu_range_read_retry()?
> 
yes.
>>   }
>>   #endif
>>   
>> @@ -997,10 +1004,18 @@ static void amdgpu_ttm_tt_unpin_userptr(struct
ttm_tt *ttm)
>>   	sg_free_table(ttm->sg);
>>   
>>   #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>> -	if (gtt->range &&
>> -	    ttm->pages[0] == hmm_device_entry_to_page(gtt->range,
>> -						      gtt->range->pfns[0]))
>> -		WARN_ONCE(1, "Missing get_user_page_done\n");
>> +	if (gtt->range) {
>> +		unsigned long i;
>> +
>> +		for (i = 0; i < ttm->num_pages; i++) {
>> +			if (ttm->pages[i] !>> +			
hmm_device_entry_to_page(gtt->range,
>> +					      gtt->range->pfns[i]))
>> +				break;
>> +		}
>> +
>> +		WARN((i == ttm->num_pages), "Missing
get_user_page_done\n");
>> +	}
> 
> Is this related/necessary? I can put it in another patch if it is just
> debugging improvement? Please advise
> 
I see this WARN backtrace now, but I didn't see it before. This is 
somehow related.
> Thanks a lot,
> Jason
>

On 2019-11-01 1:42 p.m., Jason Gunthorpe wrote:
> On Fri, Nov 01, 2019 at 03:59:26PM +0000, Yang, Philip wrote:
>>> This test for range_blockable should be before mutex_lock, I can
move
>>> it up
>>>
>> yes, thanks.
> 
> Okay, I wrote it like this:
> 
> 	if (mmu_notifier_range_blockable(range))
> 		mutex_lock(&adev->notifier_lock);
> 	else if (!mutex_trylock(&adev->notifier_lock))
> 		return false;
> 
>>> Also, do you know if notifier_lock is held while calling
>>> amdgpu_ttm_tt_get_user_pages_done()? Can we add a 'lock assert
held'
>>> to amdgpu_ttm_tt_get_user_pages_done()?
>>
>> gpu side hold notifier_lock but kfd side doesn't. kfd side
doesn't check
>> amdgpu_ttm_tt_get_user_pages_done/mmu_range_read_retry return value but
>> check mem->invalid flag which is updated from invalidate callback.
It
>> takes more time to change, I will come to another patch to fix it
later.
> 
> Ah.. confusing, OK, I'll let you sort that
> 
>>> However, this is all pre-existing bugs, so I'm OK go ahead with
this
>>> patch as modified. I advise AMD to make a followup patch ..
>>>
>> yes, I will.
> 
> While you are here, this is also wrong:
> 
> int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages)
> {
> 	down_read(&mm->mmap_sem);
> 	r = hmm_range_fault(range, 0);
> 	up_read(&mm->mmap_sem);
> 	if (unlikely(r <= 0)) {
> 		if ((r == 0 || r == -EBUSY) && !time_after(jiffies, timeout))
> 			goto retry;
> 		goto out_free_pfns;
> 	}
> 
> 	for (i = 0; i < ttm->num_pages; i++) {
> 		pages[i] = hmm_device_entry_to_page(range, range->pfns[i]);
> 
> It is not allowed to read the results of hmm_range_fault() outside
> locking, and in particular, we can't convert to a struct page.
> 
> This must be done inside the notifier_lock, after checking
> mmu_range_read_retry(), all handling of the struct page must be
> structured like that.
> 
Below change will fix this, then driver will call mmu_range_read_retry 
second time using same range->notifier_seq to check if range is 
invalidated inside amdgpu_cs_submit, this looks ok for me.

@@ -868,6 +869,13 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo 
*bo, struct page **pages)
                 goto out_free_pfns;
         }

+       mutex_lock(&adev->notifier_lock);
+
+       if (mmu_range_read_retry(&bo->notifier, range->notifier_seq))
{
+               mutex_unlock(&adev->notifier_lock);
+               goto retry;
+       }
+
         for (i = 0; i < ttm->num_pages; i++) {
                 pages[i] = hmm_device_entry_to_page(range, range->pfns[i]);
                 if (unlikely(!pages[i])) {
@@ -875,10 +883,12 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo 
*bo, struct page **pages)
                                i, range->pfns[i]);
                         r = -ENOMEM;

+                       mutex_unlock(&adev->notifier_lock);
                         goto out_free_pfns;
                 }
         }

+       mutex_unlock(&adev->notifier_lock);
         gtt->range = range;
         mmput(mm);

Philip
>>>> @@ -997,10 +1004,18 @@ static void
amdgpu_ttm_tt_unpin_userptr(struct ttm_tt *ttm)
>>>>    	sg_free_table(ttm->sg);
>>>>    
>>>>    #if IS_ENABLED(CONFIG_DRM_AMDGPU_USERPTR)
>>>> -	if (gtt->range &&
>>>> -	    ttm->pages[0] ==
hmm_device_entry_to_page(gtt->range,
>>>> -						      gtt->range->pfns[0]))
>>>> -		WARN_ONCE(1, "Missing get_user_page_done\n");
>>>> +	if (gtt->range) {
>>>> +		unsigned long i;
>>>> +
>>>> +		for (i = 0; i < ttm->num_pages; i++) {
>>>> +			if (ttm->pages[i] !>>>> +			
hmm_device_entry_to_page(gtt->range,
>>>> +					      gtt->range->pfns[i]))
>>>> +				break;
>>>> +		}
>>>> +
>>>> +		WARN((i == ttm->num_pages), "Missing
get_user_page_done\n");
>>>> +	}
>>>
>>> Is this related/necessary? I can put it in another patch if it is
just
>>> debugging improvement? Please advise
>>>
>> I see this WARN backtrace now, but I didn't see it before. This is
>> somehow related.
> 
> Hm, might be instructive to learn what is going on..
> 
> Thanks,
> Jason
>