Re: [PATCH 1/3] x86,mm/pat: Use generic interval trees

[Davidlohr Bueso <dave@stgolabs.net>]

On Wed, 04 Sep 2019, Michel Lespinasse wrote:

>I do not have time for a full review right now, but I did have a quick
>pass at it and it does seem to match the direction I'd like this to
>take.

Thanks, and no worries, I consider all this v5.5 material anyway.

>
>Please let me know if you'd like me to do a full review of this, or if
>it'll be easier to do it on the individual steps once this gets broken
>up.

If nothing else, I would appreciate you making sure I didn't do anything
stupid in the interval_tree_generic.h lookup changes.

>
>BTW are you going to plumbers ? I am and I would love to talk to you
>there, about this and about MM range locking, too.

No, not this year; perhaps lsfmm (although that's not for a while).

>
>Things I noticed in my quick pass:
>
>> diff --git a/drivers/gpu/drm/radeon/radeon_vm.c b/drivers/gpu/drm/radeon/radeon_vm.c
>> index e0ad547786e8..dc9fad8ea84a 100644
>> --- a/drivers/gpu/drm/radeon/radeon_vm.c
>> +++ b/drivers/gpu/drm/radeon/radeon_vm.c
>> @@ -450,13 +450,14 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>>  {
>>         uint64_t size = radeon_bo_size(bo_va->bo);
>>         struct radeon_vm *vm = bo_va->vm;
>> -       unsigned last_pfn, pt_idx;
>> +       unsigned pt_idx;
>>         uint64_t eoffset;
>>         int r;
>>
>>         if (soffset) {
>> +               unsigned last_pfn;
>>                 /* make sure object fit at this offset */
>> -               eoffset = soffset + size - 1;
>> +               eoffset = soffset + size;
>>                 if (soffset >= eoffset) {
>Should probably be if (soffset > eoffset) now (just checking for
>arithmetic overflow).
>>                         r = -EINVAL;
>>                         goto error_unreserve;
>> @@ -471,7 +472,7 @@ int radeon_vm_bo_set_addr(struct radeon_device *rdev,
>>                 }
>>
>>         } else {
>> -               eoffset = last_pfn = 0;
>> +               eoffset = 1; /* interval trees are [a,b[ */
>Looks highly suspicious to me, and doesn't jive with the eoffset /=
>RADEON_GPU_PAGE_SIZE below.
>I did not look deep enough into this to figure out what would be correct though.

I think you're right, I will double check this. I think we only wanna do
the RADEON_GPU_PAGE_SIZE division when soffset is non-zero.

>>         }
>>
>>         mutex_lock(&vm->mutex);
>> diff --git a/drivers/infiniband/hw/hfi1/mmu_rb.c b/drivers/infiniband/hw/hfi1/mmu_rb.c
>> index 14d2a90964c3..d708c45bfabf 100644
>> --- a/drivers/infiniband/hw/hfi1/mmu_rb.c
>> +++ b/drivers/infiniband/hw/hfi1/mmu_rb.c
>> @@ -195,13 +195,13 @@ static struct mmu_rb_node *__mmu_rb_search(struct mmu_rb_handler *handler,
>>         trace_hfi1_mmu_rb_search(addr, len);
>>         if (!handler->ops->filter) {
>>                 node = __mmu_int_rb_iter_first(&handler->root, addr,
>> -                                              (addr + len) - 1);
>> +                                              (addr + len));
>>         } else {
>>                 for (node = __mmu_int_rb_iter_first(&handler->root, addr,
>> -                                                   (addr + len) - 1);
>> +                                                   (addr + len));
>>                      node;
>>                      node = __mmu_int_rb_iter_next(node, addr,
>> -                                                  (addr + len) - 1)) {
>> +                                                  (addr + len))) {
>>                         if (handler->ops->filter(node, addr, len))
>>                                 return node;
>>                 }
>
>Weird unnecessary parentheses through this block.

Yes, but I wanted to leave it with the least amount of changes. There are plenty
of places that could use interval_tree_foreach helpers, like the vma tree has.

>
>> @@ -787,7 +787,7 @@ static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
>>         struct viommu_domain *vdomain = to_viommu_domain(domain);
>>
>>         spin_lock_irqsave(&vdomain->mappings_lock, flags);
>> -       node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
>> +       node = interval_tree_iter_first(&vdomain->mappings, iova, iova + 1);
>
>I was gonna suggest a stab iterator for the generic interval tree, but
>maybe not if it's only used here.

I considered it as well.

>
>> diff --git a/include/linux/interval_tree_generic.h b/include/linux/interval_tree_generic.h
>> index aaa8a0767aa3..e714e67ebdb5 100644
>> --- a/include/linux/interval_tree_generic.h
>> +++ b/include/linux/interval_tree_generic.h
>> @@ -69,12 +69,12 @@ ITSTATIC void ITPREFIX ## _remove(ITSTRUCT *node,                         \
>>  }                                                                            \
>>                                                                               \
>>  /*                                                                           \
>> - * Iterate over intervals intersecting [start;last]                          \
>> + * Iterate over intervals intersecting [start;last[                          \
>
>Maybe I'm extra nitpicky, but I would suggest to use 'end' instead of
>'last' when the intervals are half open: [start;end[
>To me 'last' implies that it's in the interval, while 'end' doesn't
>imply anything (and thus the half open convention used through the
>kernel applies).

That's fair enough.

>similarly ITLAST should be changed to ITEND, etc and similarly in the
>various call sites (drm and others).
>Again, maybe it's nitpicky but I feel changing the name also helps
>verify that we don't leave behind any off-by-one use.
>
>That said, it's really only my preference - if you think it's too
>painful to change, that's OK.

I'll see what I can do, but yeah it might be too much of a pain for
the benefits.

Thanks,
Davidlohr