Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register

[Ralph Campbell <rcampbell@nvidia.com>]

On 6/7/19 11:24 AM, Ralph Campbell wrote:
> 
> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>> From: Jason Gunthorpe <jgg@mellanox.com>
>>
>> Ralph observes that hmm_range_register() can only be called by a driver
>> while a mirror is registered. Make this clear in the API by passing in 
>> the
>> mirror structure as a parameter.
>>
>> This also simplifies understanding the lifetime model for struct hmm, as
>> the hmm pointer must be valid as part of a registered mirror so all we
>> need in hmm_register_range() is a simple kref_get.
>>
>> Suggested-by: Ralph Campbell <rcampbell@nvidia.com>
>> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> 
> You might CC Ben for the nouveau part.
> CC: Ben Skeggs <bskeggs@redhat.com>
> 
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> 
> 
>> ---
>> v2
>> - Include the oneline patch to nouveau_svm.c
>> ---
>>   drivers/gpu/drm/nouveau/nouveau_svm.c |  2 +-
>>   include/linux/hmm.h                   |  7 ++++---
>>   mm/hmm.c                              | 15 ++++++---------
>>   3 files changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c 
>> b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> index 93ed43c413f0bb..8c92374afcf227 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> @@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
>>           range.values = nouveau_svm_pfn_values;
>>           range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
>>   again:
>> -        ret = hmm_vma_fault(&range, true);
>> +        ret = hmm_vma_fault(&svmm->mirror, &range, true);
>>           if (ret == 0) {
>>               mutex_lock(&svmm->mutex);
>>               if (!hmm_vma_range_done(&range)) {
>> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
>> index 688c5ca7068795..2d519797cb134a 100644
>> --- a/include/linux/hmm.h
>> +++ b/include/linux/hmm.h
>> @@ -505,7 +505,7 @@ static inline bool hmm_mirror_mm_is_alive(struct 
>> hmm_mirror *mirror)
>>    * Please see Documentation/vm/hmm.rst for how to use the range API.
>>    */
>>   int hmm_range_register(struct hmm_range *range,
>> -               struct mm_struct *mm,
>> +               struct hmm_mirror *mirror,
>>                  unsigned long start,
>>                  unsigned long end,
>>                  unsigned page_shift);
>> @@ -541,7 +541,8 @@ static inline bool hmm_vma_range_done(struct 
>> hmm_range *range)
>>   }
>>   /* This is a temporary helper to avoid merge conflict between trees. */
>> -static inline int hmm_vma_fault(struct hmm_range *range, bool block)
>> +static inline int hmm_vma_fault(struct hmm_mirror *mirror,
>> +                struct hmm_range *range, bool block)
>>   {
>>       long ret;
>> @@ -554,7 +555,7 @@ static inline int hmm_vma_fault(struct hmm_range 
>> *range, bool block)
>>       range->default_flags = 0;
>>       range->pfn_flags_mask = -1UL;
>> -    ret = hmm_range_register(range, range->vma->vm_mm,
>> +    ret = hmm_range_register(range, mirror,
>>                    range->start, range->end,
>>                    PAGE_SHIFT);
>>       if (ret)
>> diff --git a/mm/hmm.c b/mm/hmm.c
>> index 547002f56a163d..8796447299023c 100644
>> --- a/mm/hmm.c
>> +++ b/mm/hmm.c
>> @@ -925,13 +925,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
>>    * Track updates to the CPU page table see include/linux/hmm.h
>>    */
>>   int hmm_range_register(struct hmm_range *range,
>> -               struct mm_struct *mm,
>> +               struct hmm_mirror *mirror,
>>                  unsigned long start,
>>                  unsigned long end,
>>                  unsigned page_shift)
>>   {
>>       unsigned long mask = ((1UL << page_shift) - 1UL);
>> -    struct hmm *hmm;
>> +    struct hmm *hmm = mirror->hmm;
>>       range->valid = false;
>>       range->hmm = NULL;
>> @@ -945,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
>>       range->start = start;
>>       range->end = end;
>> -    hmm = hmm_get_or_create(mm);
>> -    if (!hmm)
>> -        return -EFAULT;
>> -
>>       /* Check if hmm_mm_destroy() was call. */
>> -    if (hmm->mm == NULL || hmm->dead) {
>> -        hmm_put(hmm);
>> +    if (hmm->mm == NULL || hmm->dead)
>>           return -EFAULT;
>> -    }
>> +
>> +    range->hmm = hmm;
>> +    kref_get(&hmm->kref);
>>       /* Initialize range to track CPU page table updates. */
>>       mutex_lock(&hmm->lock);
>>

I forgot to add that I think you can delete the duplicate
     "range->hmm = hmm;"
here between the mutex_lock/unlock.