lkml.org 
[lkml]   [2019]   [Sep]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] x86,mm/pat: Use generic interval trees
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

\
 
 \ /
  Last update: 2019-09-05 05:47    [W:0.031 / U:1.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site