On 3/4/19 10:41 AM, ira.weiny@xxxxxxxxx wrote: > From: Ira Weiny <ira.weiny@xxxxxxxxx> > > No device supports ODP MR without an invalidate_range callback. > > Warn on any any device which attempts to support ODP without > supplying this callback. > > Then we can remove the checks for the callback within the code. > > This stems from the discussion > > https://www.spinics.net/lists/linux-rdma/msg76460.html > > ...which concluded this code was no longer necessary. > > CC: Haggai Eran <haggaie@xxxxxxxxxxxx> > CC: Artemy Kovalyov <artemyko@xxxxxxxxxxxx> > CC: John Hubbard <jhubbard@xxxxxxxxxx> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx> > --- > drivers/infiniband/core/umem.c | 5 +++++ > drivers/infiniband/core/umem_odp.c | 11 ++++------- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c > index fe5551562dbc..89a7d57f9fa5 100644 > --- a/drivers/infiniband/core/umem.c > +++ b/drivers/infiniband/core/umem.c > @@ -138,6 +138,11 @@ struct ib_umem *ib_umem_get(struct ib_udata *udata, unsigned long addr, > mmgrab(mm); > > if (access & IB_ACCESS_ON_DEMAND) { > + if (WARN_ON_ONCE(!context->invalidate_range)) { > + ret = -EINVAL; > + goto umem_kfree; > + } > + > ret = ib_umem_odp_get(to_ib_umem_odp(umem), access); > if (ret) > goto umem_kfree; > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c > index 6013cf0b8f4f..aa7f95633581 100644 > --- a/drivers/infiniband/core/umem_odp.c > +++ b/drivers/infiniband/core/umem_odp.c > @@ -240,7 +240,7 @@ static struct ib_ucontext_per_mm *alloc_per_mm(struct ib_ucontext *ctx, > per_mm->mm = mm; > per_mm->umem_tree = RB_ROOT_CACHED; > init_rwsem(&per_mm->umem_rwsem); > - per_mm->active = ctx->invalidate_range; > + per_mm->active = true; > > rcu_read_lock(); > per_mm->tgid = get_task_pid(current->group_leader, PIDTYPE_PID); > @@ -539,11 +539,10 @@ static int ib_umem_odp_map_dma_single_page( > } > > out: > - /* On Demand Paging - avoid pinning the page */ > - if (umem->context->invalidate_range || !stored_page) > + if (!stored_page) > put_page(page); Hi Ira, This would introduce a change in behavior, because previously, the first side of the "or" phrase was effectively always true. In other words, the code has, so far, been just doing this: put_page(); ...unconditionally. (Again, because "umem->context->invalidate_range" was always true. The "!stored_page" never got even evaluated.) In other words, if a == true, you cannot replace "a || b" with "b". You have to replace it with "true". I don't know what the !stored_page is about without looking more closely, but to avoid regressions you'd be safely with just doing an unconditional put_page(), I think. thanks, -- John Hubbard NVIDIA > > - if (remove_existing_mapping && umem->context->invalidate_range) { > + if (remove_existing_mapping) { > ib_umem_notifier_start_account(umem_odp); > umem->context->invalidate_range( > umem_odp, > @@ -748,9 +747,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, > */ > set_page_dirty(head_page); > } > - /* on demand pinning support */ > - if (!umem->context->invalidate_range) > - put_page(page); > + put_page(page); > umem_odp->page_list[idx] = NULL; > umem_odp->dma_list[idx] = 0; > umem->npages--; >
