Using vma_lookup() verifies the start address is contained in the found
vma. This results in easier to read the code.
Signed-off-by: Deming Wang <[email protected]>
---
drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 64fdf63093a0..cabcc2ca3c23 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1586,8 +1586,8 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
unsigned long npages;
bool readonly;
- vma = find_vma(mm, addr);
- if (!vma || addr < vma->vm_start) {
+ vma = vma_lookup(mm, addr);
+ if (!vma) {
r = -EFAULT;
goto unreserve_out;
}
@@ -2542,8 +2542,8 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
struct interval_tree_node *node;
unsigned long start_limit, end_limit;
- vma = find_vma(p->mm, addr << PAGE_SHIFT);
- if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
+ vma = vma_lookup(p->mm, addr << PAGE_SHIFT);
+ if (!vma) {
pr_debug("VMA does not exist in address [0x%llx]\n", addr);
return -EFAULT;
}
@@ -2871,8 +2871,8 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
/* __do_munmap removed VMA, return success as we are handling stale
* retry fault.
*/
- vma = find_vma(mm, addr << PAGE_SHIFT);
- if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
+ vma = vma_lookup(mm, addr << PAGE_SHIFT);
+ if (!vma) {
pr_debug("address 0x%llx VMA is removed\n", addr);
r = 0;
goto out_unlock_range;
--
2.27.0
On 2022-10-06 22:48, Deming Wang wrote:
> Using vma_lookup() verifies the start address is contained in the found
> vma. This results in easier to read the code.
Thank you for the patches. This and your other patch look good to me.
However, you missed one use of find_vma in svm_range_is_valid. Is that
an oversight or is there a reason why we need to use find_vma there?
If you're going to respin it, you may also squash the two patches into one.
Thanks,
Felix
>
> Signed-off-by: Deming Wang <[email protected]>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index 64fdf63093a0..cabcc2ca3c23 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -1586,8 +1586,8 @@ static int svm_range_validate_and_map(struct mm_struct *mm,
> unsigned long npages;
> bool readonly;
>
> - vma = find_vma(mm, addr);
> - if (!vma || addr < vma->vm_start) {
> + vma = vma_lookup(mm, addr);
> + if (!vma) {
> r = -EFAULT;
> goto unreserve_out;
> }
> @@ -2542,8 +2542,8 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
> struct interval_tree_node *node;
> unsigned long start_limit, end_limit;
>
> - vma = find_vma(p->mm, addr << PAGE_SHIFT);
> - if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> + vma = vma_lookup(p->mm, addr << PAGE_SHIFT);
> + if (!vma) {
> pr_debug("VMA does not exist in address [0x%llx]\n", addr);
> return -EFAULT;
> }
> @@ -2871,8 +2871,8 @@ svm_range_restore_pages(struct amdgpu_device *adev, unsigned int pasid,
> /* __do_munmap removed VMA, return success as we are handling stale
> * retry fault.
> */
> - vma = find_vma(mm, addr << PAGE_SHIFT);
> - if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> + vma = vma_lookup(mm, addr << PAGE_SHIFT);
> + if (!vma) {
> pr_debug("address 0x%llx VMA is removed\n", addr);
> r = 0;
> goto out_unlock_range;
Hi,
The function vma_lookup show below. Vma valid check is included in it. Or, What other questions do you have?
static inline
struct vm_area_struct *vma_lookup(struct mm_struct *mm, unsigned long addr)
{
struct vm_area_struct *vma = find_vma(mm, addr);
if (vma && addr < vma->vm_start)
vma = NULL;
return vma;
}
> from: Felix Kuehling <[email protected]>
> time: 2022年10月18日 3:35
> to: tomorrow Wang (王德明) <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> [email protected]
> sub: Re: [PATCH] drm/amdkfd: use vma_lookup() instead of find_vma()
>
>
> On 2022-10-06 22:48, Deming Wang wrote:
> > Using vma_lookup() verifies the start address is contained in the
> > found vma. This results in easier to read the code.
>
> Thank you for the patches. This and your other patch look good to me.
> However, you missed one use of find_vma in svm_range_is_valid. Is that an
> oversight or is there a reason why we need to use find_vma there?
>
> If you're going to respin it, you may also squash the two patches into one.
>
> Thanks,
> Felix
>
>
> >
> > Signed-off-by: Deming Wang <[email protected]>
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > index 64fdf63093a0..cabcc2ca3c23 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> > @@ -1586,8 +1586,8 @@ static int svm_range_validate_and_map(struct
> mm_struct *mm,
> > unsigned long npages;
> > bool readonly;
> >
> > - vma = find_vma(mm, addr);
> > - if (!vma || addr < vma->vm_start) {
> > + vma = vma_lookup(mm, addr);
> > + if (!vma) {
> > r = -EFAULT;
> > goto unreserve_out;
> > }
> > @@ -2542,8 +2542,8 @@ svm_range_get_range_boundaries(struct
> kfd_process *p, int64_t addr,
> > struct interval_tree_node *node;
> > unsigned long start_limit, end_limit;
> >
> > - vma = find_vma(p->mm, addr << PAGE_SHIFT);
> > - if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> > + vma = vma_lookup(p->mm, addr << PAGE_SHIFT);
> > + if (!vma) {
> > pr_debug("VMA does not exist in address [0x%llx]\n", addr);
> > return -EFAULT;
> > }
> > @@ -2871,8 +2871,8 @@ svm_range_restore_pages(struct amdgpu_device
> *adev, unsigned int pasid,
> > /* __do_munmap removed VMA, return success as we are handling stale
> > * retry fault.
> > */
> > - vma = find_vma(mm, addr << PAGE_SHIFT);
> > - if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
> > + vma = vma_lookup(mm, addr << PAGE_SHIFT);
> > + if (!vma) {
> > pr_debug("address 0x%llx VMA is removed\n", addr);
> > r = 0;
> > goto out_unlock_range;
Am 2022-10-17 um 20:47 schrieb tomorrow Wang (王德明):
> Hi,
> The function vma_lookup show below. Vma valid check is included in it. Or, What other questions do you have?
My question is, why did you leave the find_vma call in
svm_range_is_valid unchanged? I don't see a technical reason, but maybe
I'm missing something. If there is a reason, please explain. If there is
no reason, please fix that place as well for consistency.
Thanks,
Felix
>
> static inline
> struct vm_area_struct *vma_lookup(struct mm_struct *mm, unsigned long addr)
> {
> struct vm_area_struct *vma = find_vma(mm, addr);
>
> if (vma && addr < vma->vm_start)
> vma = NULL;
>
> return vma;
> }
>
>
>> from: Felix Kuehling <[email protected]>
>> time: 2022年10月18日 3:35
>> to: tomorrow Wang (王德明) <[email protected]>;
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]
>> [email protected]
>> sub: Re: [PATCH] drm/amdkfd: use vma_lookup() instead of find_vma()
>>
>>
>> On 2022-10-06 22:48, Deming Wang wrote:
>>> Using vma_lookup() verifies the start address is contained in the
>>> found vma. This results in easier to read the code.
>> Thank you for the patches. This and your other patch look good to me.
>> However, you missed one use of find_vma in svm_range_is_valid. Is that an
>> oversight or is there a reason why we need to use find_vma there?
>>
>> If you're going to respin it, you may also squash the two patches into one.
>>
>> Thanks,
>> Felix
>>
>>
>>> Signed-off-by: Deming Wang <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index 64fdf63093a0..cabcc2ca3c23 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -1586,8 +1586,8 @@ static int svm_range_validate_and_map(struct
>> mm_struct *mm,
>>> unsigned long npages;
>>> bool readonly;
>>>
>>> - vma = find_vma(mm, addr);
>>> - if (!vma || addr < vma->vm_start) {
>>> + vma = vma_lookup(mm, addr);
>>> + if (!vma) {
>>> r = -EFAULT;
>>> goto unreserve_out;
>>> }
>>> @@ -2542,8 +2542,8 @@ svm_range_get_range_boundaries(struct
>> kfd_process *p, int64_t addr,
>>> struct interval_tree_node *node;
>>> unsigned long start_limit, end_limit;
>>>
>>> - vma = find_vma(p->mm, addr << PAGE_SHIFT);
>>> - if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>> + vma = vma_lookup(p->mm, addr << PAGE_SHIFT);
>>> + if (!vma) {
>>> pr_debug("VMA does not exist in address [0x%llx]\n", addr);
>>> return -EFAULT;
>>> }
>>> @@ -2871,8 +2871,8 @@ svm_range_restore_pages(struct amdgpu_device
>> *adev, unsigned int pasid,
>>> /* __do_munmap removed VMA, return success as we are handling stale
>>> * retry fault.
>>> */
>>> - vma = find_vma(mm, addr << PAGE_SHIFT);
>>> - if (!vma || (addr << PAGE_SHIFT) < vma->vm_start) {
>>> + vma = vma_lookup(mm, addr << PAGE_SHIFT);
>>> + if (!vma) {
>>> pr_debug("address 0x%llx VMA is removed\n", addr);
>>> r = 0;
>>> goto out_unlock_range;