| Message ID | 20191003201858.11666-6-dave@stgolabs.net (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | lib/interval-tree: move to half closed intervals | expand |
On Thu, Oct 03, 2019 at 01:18:52PM -0700, Davidlohr Bueso wrote: > diff --git a/drivers/infiniband/hw/hfi1/mmu_rb.c b/drivers/infiniband/hw/hfi1/mmu_rb.c > index 14d2a90964c3..fb6382b2d44e 100644 > --- a/drivers/infiniband/hw/hfi1/mmu_rb.c > +++ b/drivers/infiniband/hw/hfi1/mmu_rb.c > @@ -47,7 +47,7 @@ > #include <linux/list.h> > #include <linux/rculist.h> > #include <linux/mmu_notifier.h> > -#include <linux/interval_tree_generic.h> > +#include <linux/interval_tree_gen.h> > > #include "mmu_rb.h" > #include "trace.h" > @@ -89,7 +89,7 @@ static unsigned long mmu_node_start(struct mmu_rb_node *node) > > static unsigned long mmu_node_last(struct mmu_rb_node *node) > { > - return PAGE_ALIGN(node->addr + node->len) - 1; > + return PAGE_ALIGN(node->addr + node->len); > } May as well rename the function mmu_node_end(). I was worried if it was used anywhere else, but it turned out it's only used when defining the interval tree. I would also suggest moving this function (as well as mmu_node_first) right before its use, rather than just after, which would allow you to also remove the function prototype a few lines earlier. Looks good to me otherwise. Reviewed-by: Michel Lespinasse <walken@google.com>
On Fri, 04 Oct 2019, Michel Lespinasse wrote: >On Thu, Oct 03, 2019 at 01:18:52PM -0700, Davidlohr Bueso wrote: >> diff --git a/drivers/infiniband/hw/hfi1/mmu_rb.c b/drivers/infiniband/hw/hfi1/mmu_rb.c >> index 14d2a90964c3..fb6382b2d44e 100644 >> --- a/drivers/infiniband/hw/hfi1/mmu_rb.c >> +++ b/drivers/infiniband/hw/hfi1/mmu_rb.c >> @@ -47,7 +47,7 @@ >> #include <linux/list.h> >> #include <linux/rculist.h> >> #include <linux/mmu_notifier.h> >> -#include <linux/interval_tree_generic.h> >> +#include <linux/interval_tree_gen.h> >> >> #include "mmu_rb.h" >> #include "trace.h" >> @@ -89,7 +89,7 @@ static unsigned long mmu_node_start(struct mmu_rb_node *node) >> >> static unsigned long mmu_node_last(struct mmu_rb_node *node) >> { >> - return PAGE_ALIGN(node->addr + node->len) - 1; >> + return PAGE_ALIGN(node->addr + node->len); >> } > >May as well rename the function mmu_node_end(). I was worried if it >was used anywhere else, but it turned out it's only used when defining >the interval tree. Right. In general I tried not to rename everything to end because I wanted to avoid bloating the diffstat, albeit having naming discrepancies within the code (which isn't new either fwiw). > >I would also suggest moving this function (as well as mmu_node_first) >right before its use, rather than just after, which would allow you to >also remove the function prototype a few lines earlier. Indeed, but again I don't want to unnecessarily grow the patch. I have several notes to come back to once/if this series is settled. > >Looks good to me otherwise. > >Reviewed-by: Michel Lespinasse <walken@google.com> Thanks, Davidlohr
diff --git a/drivers/infiniband/hw/hfi1/mmu_rb.c b/drivers/infiniband/hw/hfi1/mmu_rb.c index 14d2a90964c3..fb6382b2d44e 100644 --- a/drivers/infiniband/hw/hfi1/mmu_rb.c +++ b/drivers/infiniband/hw/hfi1/mmu_rb.c @@ -47,7 +47,7 @@ #include <linux/list.h> #include <linux/rculist.h> #include <linux/mmu_notifier.h> -#include <linux/interval_tree_generic.h> +#include <linux/interval_tree_gen.h> #include "mmu_rb.h" #include "trace.h" @@ -89,7 +89,7 @@ static unsigned long mmu_node_start(struct mmu_rb_node *node) static unsigned long mmu_node_last(struct mmu_rb_node *node) { - return PAGE_ALIGN(node->addr + node->len) - 1; + return PAGE_ALIGN(node->addr + node->len); } int hfi1_mmu_rb_register(void *ops_arg, struct mm_struct *mm, @@ -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; } @@ -293,11 +293,10 @@ static int mmu_notifier_range_start(struct mmu_notifier *mn, bool added = false; spin_lock_irqsave(&handler->lock, flags); - for (node = __mmu_int_rb_iter_first(root, range->start, range->end-1); + for (node = __mmu_int_rb_iter_first(root, range->start, range->end); node; node = ptr) { /* Guard against node removal. */ - ptr = __mmu_int_rb_iter_next(node, range->start, - range->end - 1); + ptr = __mmu_int_rb_iter_next(node, range->start, range->end); trace_hfi1_mmu_mem_invalidate(node->addr, node->len); if (handler->ops->invalidate(handler->ops_arg, node)) { __mmu_int_rb_remove(node, root);
The __mmu_int_rb interval tree really wants [a, b) intervals, not fully closed as currently. As such convert it to use the new interval_tree_gen.h. Cc: Mike Marciniszyn <mike.marciniszyn@intel.com> Cc: Dennis Dalessandro <dennis.dalessandro@intel.com> Cc: Doug Ledford <dledford@redhat.com> Cc: linux-rdma@vger.kernel.org Signed-off-by: Davidlohr Bueso <dbueso@suse.de> --- drivers/infiniband/hw/hfi1/mmu_rb.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)