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
