Messages in this thread | | | Date | Wed, 4 Sep 2019 20:45:46 -0700 | From | Davidlohr Bueso <> | Subject | Re: [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
| |