| Message ID | 20191028201032.6352-8-jgg@ziepe.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Consolidate the mmu notifier interval_tree and locking | expand |
Am 28.10.19 um 21:10 schrieb Jason Gunthorpe: > From: Jason Gunthorpe <jgg@mellanox.com> > > The new API is an exact match for the needs of radeon. > > For some reason radeon tries to remove overlapping ranges from the > interval tree, but interval trees (and mmu_range_notifier_insert) > support overlapping ranges directly. Simply delete all this code. > > Since this driver is missing a invalidate_range_end callback, but > still calls get_user_pages(), it cannot be correct against all races. > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: David (ChunMing) Zhou <David1.Zhou@amd.com> > Cc: amd-gfx@lists.freedesktop.org > Cc: Petr Cvek <petrcvekcz@gmail.com> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/radeon/radeon.h | 9 +- > drivers/gpu/drm/radeon/radeon_mn.c | 219 ++++++----------------------- > 2 files changed, 52 insertions(+), 176 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h > index d59b004f669583..27959f3ace1152 100644 > --- a/drivers/gpu/drm/radeon/radeon.h > +++ b/drivers/gpu/drm/radeon/radeon.h > @@ -68,6 +68,10 @@ > #include <linux/hashtable.h> > #include <linux/dma-fence.h> > > +#ifdef CONFIG_MMU_NOTIFIER > +#include <linux/mmu_notifier.h> > +#endif > + > #include <drm/ttm/ttm_bo_api.h> > #include <drm/ttm/ttm_bo_driver.h> > #include <drm/ttm/ttm_placement.h> > @@ -509,8 +513,9 @@ struct radeon_bo { > struct ttm_bo_kmap_obj dma_buf_vmap; > pid_t pid; > > - struct radeon_mn *mn; > - struct list_head mn_list; > +#ifdef CONFIG_MMU_NOTIFIER > + struct mmu_range_notifier notifier; > +#endif > }; > #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, tbo.base) > > diff --git a/drivers/gpu/drm/radeon/radeon_mn.c b/drivers/gpu/drm/radeon/radeon_mn.c > index dbab9a3a969b9e..d3d41e20a64922 100644 > --- a/drivers/gpu/drm/radeon/radeon_mn.c > +++ b/drivers/gpu/drm/radeon/radeon_mn.c > @@ -36,131 +36,51 @@ > > #include "radeon.h" > > -struct radeon_mn { > - struct mmu_notifier mn; > - > - /* objects protected by lock */ > - struct mutex lock; > - struct rb_root_cached objects; > -}; > - > -struct radeon_mn_node { > - struct interval_tree_node it; > - struct list_head bos; > -}; > - > /** > - * radeon_mn_invalidate_range_start - callback to notify about mm change > + * radeon_mn_invalidate - callback to notify about mm change > * > * @mn: our notifier > - * @mn: the mm this callback is about > - * @start: start of updated range > - * @end: end of updated range > + * @range: the VMA under invalidation > * > * We block for all BOs between start and end to be idle and > * unmap them by move them into system domain again. > */ > -static int radeon_mn_invalidate_range_start(struct mmu_notifier *mn, > - const struct mmu_notifier_range *range) > +static bool radeon_mn_invalidate(struct mmu_range_notifier *mn, > + const struct mmu_notifier_range *range, > + unsigned long cur_seq) > { > - struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn); > + struct radeon_bo *bo = container_of(mn, struct radeon_bo, notifier); > struct ttm_operation_ctx ctx = { false, false }; > - struct interval_tree_node *it; > - unsigned long end; > - int ret = 0; > - > - /* notification is exclusive, but interval is inclusive */ > - end = range->end - 1; > - > - /* TODO we should be able to split locking for interval tree and > - * the tear down. > - */ > - if (mmu_notifier_range_blockable(range)) > - mutex_lock(&rmn->lock); > - else if (!mutex_trylock(&rmn->lock)) > - return -EAGAIN; > - > - it = interval_tree_iter_first(&rmn->objects, range->start, end); > - while (it) { > - struct radeon_mn_node *node; > - struct radeon_bo *bo; > - long r; > - > - if (!mmu_notifier_range_blockable(range)) { > - ret = -EAGAIN; > - goto out_unlock; > - } > - > - node = container_of(it, struct radeon_mn_node, it); > - it = interval_tree_iter_next(it, range->start, end); > + long r; > > - list_for_each_entry(bo, &node->bos, mn_list) { > + if (!bo->tbo.ttm || bo->tbo.ttm->state != tt_bound) > + return true; > > - if (!bo->tbo.ttm || bo->tbo.ttm->state != tt_bound) > - continue; > + if (!mmu_notifier_range_blockable(range)) > + return false; > > - r = radeon_bo_reserve(bo, true); > - if (r) { > - DRM_ERROR("(%ld) failed to reserve user bo\n", r); > - continue; > - } > - > - r = dma_resv_wait_timeout_rcu(bo->tbo.base.resv, > - true, false, MAX_SCHEDULE_TIMEOUT); > - if (r <= 0) > - DRM_ERROR("(%ld) failed to wait for user bo\n", r); > - > - radeon_ttm_placement_from_domain(bo, RADEON_GEM_DOMAIN_CPU); > - r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > - if (r) > - DRM_ERROR("(%ld) failed to validate user bo\n", r); > - > - radeon_bo_unreserve(bo); > - } > + r = radeon_bo_reserve(bo, true); > + if (r) { > + DRM_ERROR("(%ld) failed to reserve user bo\n", r); > + return true; > } > - > -out_unlock: > - mutex_unlock(&rmn->lock); > - > - return ret; > -} > - > -static void radeon_mn_release(struct mmu_notifier *mn, struct mm_struct *mm) > -{ > - struct mmu_notifier_range range = { > - .mm = mm, > - .start = 0, > - .end = ULONG_MAX, > - .flags = 0, > - .event = MMU_NOTIFY_UNMAP, > - }; > - > - radeon_mn_invalidate_range_start(mn, &range); > -} > - > -static struct mmu_notifier *radeon_mn_alloc_notifier(struct mm_struct *mm) > -{ > - struct radeon_mn *rmn; > > - rmn = kzalloc(sizeof(*rmn), GFP_KERNEL); > - if (!rmn) > - return ERR_PTR(-ENOMEM); > + r = dma_resv_wait_timeout_rcu(bo->tbo.base.resv, true, false, > + MAX_SCHEDULE_TIMEOUT); > + if (r <= 0) > + DRM_ERROR("(%ld) failed to wait for user bo\n", r); > > - mutex_init(&rmn->lock); > - rmn->objects = RB_ROOT_CACHED; > - return &rmn->mn; > -} > + radeon_ttm_placement_from_domain(bo, RADEON_GEM_DOMAIN_CPU); > + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > + if (r) > + DRM_ERROR("(%ld) failed to validate user bo\n", r); > > -static void radeon_mn_free_notifier(struct mmu_notifier *mn) > -{ > - kfree(container_of(mn, struct radeon_mn, mn)); > + radeon_bo_unreserve(bo); > + return true; > } > > -static const struct mmu_notifier_ops radeon_mn_ops = { > - .release = radeon_mn_release, > - .invalidate_range_start = radeon_mn_invalidate_range_start, > - .alloc_notifier = radeon_mn_alloc_notifier, > - .free_notifier = radeon_mn_free_notifier, > +static const struct mmu_range_notifier_ops radeon_mn_ops = { > + .invalidate = radeon_mn_invalidate, > }; > > /** > @@ -174,51 +94,21 @@ static const struct mmu_notifier_ops radeon_mn_ops = { > */ > int radeon_mn_register(struct radeon_bo *bo, unsigned long addr) > { > - unsigned long end = addr + radeon_bo_size(bo) - 1; > - struct mmu_notifier *mn; > - struct radeon_mn *rmn; > - struct radeon_mn_node *node = NULL; > - struct list_head bos; > - struct interval_tree_node *it; > - > - mn = mmu_notifier_get(&radeon_mn_ops, current->mm); > - if (IS_ERR(mn)) > - return PTR_ERR(mn); > - rmn = container_of(mn, struct radeon_mn, mn); > - > - INIT_LIST_HEAD(&bos); > - > - mutex_lock(&rmn->lock); > - > - while ((it = interval_tree_iter_first(&rmn->objects, addr, end))) { > - kfree(node); > - node = container_of(it, struct radeon_mn_node, it); > - interval_tree_remove(&node->it, &rmn->objects); > - addr = min(it->start, addr); > - end = max(it->last, end); > - list_splice(&node->bos, &bos); > - } > - > - if (!node) { > - node = kmalloc(sizeof(struct radeon_mn_node), GFP_KERNEL); > - if (!node) { > - mutex_unlock(&rmn->lock); > - return -ENOMEM; > - } > - } > - > - bo->mn = rmn; > - > - node->it.start = addr; > - node->it.last = end; > - INIT_LIST_HEAD(&node->bos); > - list_splice(&bos, &node->bos); > - list_add(&bo->mn_list, &node->bos); > - > - interval_tree_insert(&node->it, &rmn->objects); > - > - mutex_unlock(&rmn->lock); > - > + int ret; > + > + bo->notifier.ops = &radeon_mn_ops; > + ret = mmu_range_notifier_insert(&bo->notifier, addr, radeon_bo_size(bo), > + current->mm); > + if (ret) > + return ret; > + > + /* > + * FIXME: radeon appears to allow get_user_pages to run during > + * invalidate_range_start/end, which is not a safe way to read the > + * PTEs. It should use the mmu_range_read_begin() scheme around the > + * get_user_pages to ensure that the PTEs are read properly > + */ > + mmu_range_read_begin(&bo->notifier); > return 0; > } > > @@ -231,27 +121,8 @@ int radeon_mn_register(struct radeon_bo *bo, unsigned long addr) > */ > void radeon_mn_unregister(struct radeon_bo *bo) > { > - struct radeon_mn *rmn = bo->mn; > - struct list_head *head; > - > - if (!rmn) > + if (!bo->notifier.mm) > return; > - > - mutex_lock(&rmn->lock); > - /* save the next list entry for later */ > - head = bo->mn_list.next; > - > - list_del(&bo->mn_list); > - > - if (list_empty(head)) { > - struct radeon_mn_node *node; > - node = container_of(head, struct radeon_mn_node, bos); > - interval_tree_remove(&node->it, &rmn->objects); > - kfree(node); > - } > - > - mutex_unlock(&rmn->lock); > - > - mmu_notifier_put(&rmn->mn); > - bo->mn = NULL; > + mmu_range_notifier_remove(&bo->notifier); > + bo->notifier.mm = NULL; > }
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index d59b004f669583..27959f3ace1152 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -68,6 +68,10 @@ #include <linux/hashtable.h> #include <linux/dma-fence.h> +#ifdef CONFIG_MMU_NOTIFIER +#include <linux/mmu_notifier.h> +#endif + #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h> #include <drm/ttm/ttm_placement.h> @@ -509,8 +513,9 @@ struct radeon_bo { struct ttm_bo_kmap_obj dma_buf_vmap; pid_t pid; - struct radeon_mn *mn; - struct list_head mn_list; +#ifdef CONFIG_MMU_NOTIFIER + struct mmu_range_notifier notifier; +#endif }; #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, tbo.base) diff --git a/drivers/gpu/drm/radeon/radeon_mn.c b/drivers/gpu/drm/radeon/radeon_mn.c index dbab9a3a969b9e..d3d41e20a64922 100644 --- a/drivers/gpu/drm/radeon/radeon_mn.c +++ b/drivers/gpu/drm/radeon/radeon_mn.c @@ -36,131 +36,51 @@ #include "radeon.h" -struct radeon_mn { - struct mmu_notifier mn; - - /* objects protected by lock */ - struct mutex lock; - struct rb_root_cached objects; -}; - -struct radeon_mn_node { - struct interval_tree_node it; - struct list_head bos; -}; - /** - * radeon_mn_invalidate_range_start - callback to notify about mm change + * radeon_mn_invalidate - callback to notify about mm change * * @mn: our notifier - * @mn: the mm this callback is about - * @start: start of updated range - * @end: end of updated range + * @range: the VMA under invalidation * * We block for all BOs between start and end to be idle and * unmap them by move them into system domain again. */ -static int radeon_mn_invalidate_range_start(struct mmu_notifier *mn, - const struct mmu_notifier_range *range) +static bool radeon_mn_invalidate(struct mmu_range_notifier *mn, + const struct mmu_notifier_range *range, + unsigned long cur_seq) { - struct radeon_mn *rmn = container_of(mn, struct radeon_mn, mn); + struct radeon_bo *bo = container_of(mn, struct radeon_bo, notifier); struct ttm_operation_ctx ctx = { false, false }; - struct interval_tree_node *it; - unsigned long end; - int ret = 0; - - /* notification is exclusive, but interval is inclusive */ - end = range->end - 1; - - /* TODO we should be able to split locking for interval tree and - * the tear down. - */ - if (mmu_notifier_range_blockable(range)) - mutex_lock(&rmn->lock); - else if (!mutex_trylock(&rmn->lock)) - return -EAGAIN; - - it = interval_tree_iter_first(&rmn->objects, range->start, end); - while (it) { - struct radeon_mn_node *node; - struct radeon_bo *bo; - long r; - - if (!mmu_notifier_range_blockable(range)) { - ret = -EAGAIN; - goto out_unlock; - } - - node = container_of(it, struct radeon_mn_node, it); - it = interval_tree_iter_next(it, range->start, end); + long r; - list_for_each_entry(bo, &node->bos, mn_list) { + if (!bo->tbo.ttm || bo->tbo.ttm->state != tt_bound) + return true; - if (!bo->tbo.ttm || bo->tbo.ttm->state != tt_bound) - continue; + if (!mmu_notifier_range_blockable(range)) + return false; - r = radeon_bo_reserve(bo, true); - if (r) { - DRM_ERROR("(%ld) failed to reserve user bo\n", r); - continue; - } - - r = dma_resv_wait_timeout_rcu(bo->tbo.base.resv, - true, false, MAX_SCHEDULE_TIMEOUT); - if (r <= 0) - DRM_ERROR("(%ld) failed to wait for user bo\n", r); - - radeon_ttm_placement_from_domain(bo, RADEON_GEM_DOMAIN_CPU); - r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); - if (r) - DRM_ERROR("(%ld) failed to validate user bo\n", r); - - radeon_bo_unreserve(bo); - } + r = radeon_bo_reserve(bo, true); + if (r) { + DRM_ERROR("(%ld) failed to reserve user bo\n", r); + return true; } - -out_unlock: - mutex_unlock(&rmn->lock); - - return ret; -} - -static void radeon_mn_release(struct mmu_notifier *mn, struct mm_struct *mm) -{ - struct mmu_notifier_range range = { - .mm = mm, - .start = 0, - .end = ULONG_MAX, - .flags = 0, - .event = MMU_NOTIFY_UNMAP, - }; - - radeon_mn_invalidate_range_start(mn, &range); -} - -static struct mmu_notifier *radeon_mn_alloc_notifier(struct mm_struct *mm) -{ - struct radeon_mn *rmn; - rmn = kzalloc(sizeof(*rmn), GFP_KERNEL); - if (!rmn) - return ERR_PTR(-ENOMEM); + r = dma_resv_wait_timeout_rcu(bo->tbo.base.resv, true, false, + MAX_SCHEDULE_TIMEOUT); + if (r <= 0) + DRM_ERROR("(%ld) failed to wait for user bo\n", r); - mutex_init(&rmn->lock); - rmn->objects = RB_ROOT_CACHED; - return &rmn->mn; -} + radeon_ttm_placement_from_domain(bo, RADEON_GEM_DOMAIN_CPU); + r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); + if (r) + DRM_ERROR("(%ld) failed to validate user bo\n", r); -static void radeon_mn_free_notifier(struct mmu_notifier *mn) -{ - kfree(container_of(mn, struct radeon_mn, mn)); + radeon_bo_unreserve(bo); + return true; } -static const struct mmu_notifier_ops radeon_mn_ops = { - .release = radeon_mn_release, - .invalidate_range_start = radeon_mn_invalidate_range_start, - .alloc_notifier = radeon_mn_alloc_notifier, - .free_notifier = radeon_mn_free_notifier, +static const struct mmu_range_notifier_ops radeon_mn_ops = { + .invalidate = radeon_mn_invalidate, }; /** @@ -174,51 +94,21 @@ static const struct mmu_notifier_ops radeon_mn_ops = { */ int radeon_mn_register(struct radeon_bo *bo, unsigned long addr) { - unsigned long end = addr + radeon_bo_size(bo) - 1; - struct mmu_notifier *mn; - struct radeon_mn *rmn; - struct radeon_mn_node *node = NULL; - struct list_head bos; - struct interval_tree_node *it; - - mn = mmu_notifier_get(&radeon_mn_ops, current->mm); - if (IS_ERR(mn)) - return PTR_ERR(mn); - rmn = container_of(mn, struct radeon_mn, mn); - - INIT_LIST_HEAD(&bos); - - mutex_lock(&rmn->lock); - - while ((it = interval_tree_iter_first(&rmn->objects, addr, end))) { - kfree(node); - node = container_of(it, struct radeon_mn_node, it); - interval_tree_remove(&node->it, &rmn->objects); - addr = min(it->start, addr); - end = max(it->last, end); - list_splice(&node->bos, &bos); - } - - if (!node) { - node = kmalloc(sizeof(struct radeon_mn_node), GFP_KERNEL); - if (!node) { - mutex_unlock(&rmn->lock); - return -ENOMEM; - } - } - - bo->mn = rmn; - - node->it.start = addr; - node->it.last = end; - INIT_LIST_HEAD(&node->bos); - list_splice(&bos, &node->bos); - list_add(&bo->mn_list, &node->bos); - - interval_tree_insert(&node->it, &rmn->objects); - - mutex_unlock(&rmn->lock); - + int ret; + + bo->notifier.ops = &radeon_mn_ops; + ret = mmu_range_notifier_insert(&bo->notifier, addr, radeon_bo_size(bo), + current->mm); + if (ret) + return ret; + + /* + * FIXME: radeon appears to allow get_user_pages to run during + * invalidate_range_start/end, which is not a safe way to read the + * PTEs. It should use the mmu_range_read_begin() scheme around the + * get_user_pages to ensure that the PTEs are read properly + */ + mmu_range_read_begin(&bo->notifier); return 0; } @@ -231,27 +121,8 @@ int radeon_mn_register(struct radeon_bo *bo, unsigned long addr) */ void radeon_mn_unregister(struct radeon_bo *bo) { - struct radeon_mn *rmn = bo->mn; - struct list_head *head; - - if (!rmn) + if (!bo->notifier.mm) return; - - mutex_lock(&rmn->lock); - /* save the next list entry for later */ - head = bo->mn_list.next; - - list_del(&bo->mn_list); - - if (list_empty(head)) { - struct radeon_mn_node *node; - node = container_of(head, struct radeon_mn_node, bos); - interval_tree_remove(&node->it, &rmn->objects); - kfree(node); - } - - mutex_unlock(&rmn->lock); - - mmu_notifier_put(&rmn->mn); - bo->mn = NULL; + mmu_range_notifier_remove(&bo->notifier); + bo->notifier.mm = NULL; }