[PATCH v2 3/3] drm/xe: Add staging tree for VM binds
Thomas Hellström
thomas.hellstrom at linux.intel.com
Mon Feb 24 09:22:37 UTC 2025
Hi,
On Sun, 2025-02-23 at 20:05 -0800, Matthew Brost wrote:
> Concurrent VM bind staging and zapping of PTEs from a userptr
> notifier
> do not work because the view of PTEs is not stable. VM binds cannot
> acquire the notifier lock during staging, as memory allocations are
> required. To resolve this race condition, use a staging tree for VM
> binds that is committed only under the userptr notifier lock during
> the
> final step of the bind. This ensures a consistent view of the PTEs in
> the userptr notifier.
>
> A follow up may only use staging for VM in fault mode as this is the
> only mode in which the above race exists.
>
> Suggested-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> Cc: <stable at vger.kernel.org>
> Fixes: e8babb280b5e ("drm/xe: Convert multiple bind ops into single
> job")
> Fixes: a708f6501c69 ("drm/xe: Update PT layer with better error
> handling")
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
This looks even simpler than what I initially thought. Ideally I think
the staging tree would exist only under the vm lock during the multi
bind-unbind operation, but that adds code complication and execution
time compared to this solution which is more memory-hungry.
Minor comment below.
> ---
> drivers/gpu/drm/xe/xe_pt.c | 50 +++++++++++++++++++++++++------
> --
> drivers/gpu/drm/xe/xe_pt_walk.c | 3 +-
> drivers/gpu/drm/xe/xe_pt_walk.h | 4 +++
> 3 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index add521b5c6ae..118b81cd7205 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -28,6 +28,8 @@ struct xe_pt_dir {
> struct xe_pt pt;
> /** @children: Array of page-table child nodes */
> struct xe_ptw *children[XE_PDES];
> + /** @staging: Array of page-table staging nodes */
> + struct xe_ptw *staging[XE_PDES];
> };
>
> #if IS_ENABLED(CONFIG_DRM_XE_DEBUG_VM)
> @@ -50,7 +52,7 @@ static struct xe_pt_dir *as_xe_pt_dir(struct xe_pt
> *pt)
>
> static struct xe_pt *xe_pt_entry(struct xe_pt_dir *pt_dir, unsigned
> int index)
> {
> - return container_of(pt_dir->children[index], struct xe_pt,
> base);
> + return container_of(pt_dir->staging[index], struct xe_pt,
> base);
It looks like the only user here is with staging == true, but I'm a
little worried about future users. Perhaps include a staging bool in
the interface and assert that it is true? Or rename to
xe_pt_entry_staging()?
> }
>
> static u64 __xe_pt_empty_pte(struct xe_tile *tile, struct xe_vm *vm,
> @@ -125,6 +127,7 @@ struct xe_pt *xe_pt_create(struct xe_vm *vm,
> struct xe_tile *tile,
> }
> pt->bo = bo;
> pt->base.children = level ? as_xe_pt_dir(pt)->children :
> NULL;
> + pt->base.staging = level ? as_xe_pt_dir(pt)->staging : NULL;
>
> if (vm->xef)
> xe_drm_client_add_bo(vm->xef->client, pt->bo);
> @@ -377,8 +380,10 @@ xe_pt_insert_entry(struct xe_pt_stage_bind_walk
> *xe_walk, struct xe_pt *parent,
> /* Continue building a non-connected subtree. */
> struct iosys_map *map = &parent->bo->vmap;
>
> - if (unlikely(xe_child))
> + if (unlikely(xe_child)) {
> parent->base.children[offset] = &xe_child-
> >base;
> + parent->base.staging[offset] = &xe_child-
> >base;
> + }
>
> xe_pt_write(xe_walk->vm->xe, map, offset, pte);
> parent->num_live++;
> @@ -619,6 +624,7 @@ xe_pt_stage_bind(struct xe_tile *tile, struct
> xe_vma *vma,
> .ops = &xe_pt_stage_bind_ops,
> .shifts = xe_normal_pt_shifts,
> .max_level = XE_PT_HIGHEST_LEVEL,
> + .staging = true,
> },
> .vm = xe_vma_vm(vma),
> .tile = tile,
> @@ -812,6 +818,7 @@ static const struct xe_pt_walk_ops
> xe_pt_zap_ptes_ops = {
>
> struct xe_pt_zap_ptes_flags {
> bool scratch:1;
> + bool staging:1;
> };
As mentioned in the comments to the previous commit. I don't think
zapping is needed when aborting a bind.
Otherwise LGTM
/Thomas
>
> static bool __xe_pt_zap_ptes(struct xe_tile *tile, struct xe_vma
> *vma,
> @@ -822,6 +829,7 @@ static bool __xe_pt_zap_ptes(struct xe_tile
> *tile, struct xe_vma *vma,
> .ops = &xe_pt_zap_ptes_ops,
> .shifts = xe_normal_pt_shifts,
> .max_level = XE_PT_HIGHEST_LEVEL,
> + .staging = flags.staging,
> },
> .tile = tile,
> .vm = xe_vma_vm(vma),
> @@ -905,7 +913,7 @@ static void xe_pt_cancel_bind(struct xe_vma *vma,
> }
> }
>
> -static void xe_pt_commit_locks_assert(struct xe_vma *vma)
> +static void xe_pt_commit_prepare_locks_assert(struct xe_vma *vma)
> {
> struct xe_vm *vm = xe_vma_vm(vma);
>
> @@ -917,6 +925,16 @@ static void xe_pt_commit_locks_assert(struct
> xe_vma *vma)
> xe_vm_assert_held(vm);
> }
>
> +static void xe_pt_commit_locks_assert(struct xe_vma *vma)
> +{
> + struct xe_vm *vm = xe_vma_vm(vma);
> +
> + xe_pt_commit_prepare_locks_assert(vma);
> +
> + if (xe_vma_is_userptr(vma))
> + lockdep_assert_held_read(&vm-
> >userptr.notifier_lock);
> +}
> +
> static void xe_pt_commit(struct xe_vma *vma,
> struct xe_vm_pgtable_update *entries,
> u32 num_entries, struct llist_head
> *deferred)
> @@ -927,13 +945,17 @@ static void xe_pt_commit(struct xe_vma *vma,
>
> for (i = 0; i < num_entries; i++) {
> struct xe_pt *pt = entries[i].pt;
> + struct xe_pt_dir *pt_dir;
>
> if (!pt->level)
> continue;
>
> + pt_dir = as_xe_pt_dir(pt);
> for (j = 0; j < entries[i].qwords; j++) {
> struct xe_pt *oldpte =
> entries[i].pt_entries[j].pt;
> + int j_ = j + entries[i].ofs;
>
> + pt_dir->children[j_] = pt_dir->staging[j_];
> xe_pt_destroy(oldpte, xe_vma_vm(vma)->flags,
> deferred);
> }
> }
> @@ -945,7 +967,7 @@ static void xe_pt_abort_bind(struct xe_vma *vma,
> {
> int i, j;
>
> - xe_pt_commit_locks_assert(vma);
> + xe_pt_commit_prepare_locks_assert(vma);
>
> for (i = num_entries - 1; i >= 0; --i) {
> struct xe_pt *pt = entries[i].pt;
> @@ -963,7 +985,7 @@ static void xe_pt_abort_bind(struct xe_vma *vma,
> struct xe_pt *newpte = xe_pt_entry(pt_dir,
> j_);
> struct xe_pt *oldpte =
> entries[i].pt_entries[j].pt;
>
> - pt_dir->children[j_] = oldpte ? &oldpte-
> >base : 0;
> + pt_dir->staging[j_] = oldpte ? &oldpte->base
> : 0;
> xe_pt_destroy(newpte, xe_vma_vm(vma)->flags,
> NULL);
> }
> }
> @@ -975,7 +997,7 @@ static void xe_pt_commit_prepare_bind(struct
> xe_vma *vma,
> {
> u32 i, j;
>
> - xe_pt_commit_locks_assert(vma);
> + xe_pt_commit_prepare_locks_assert(vma);
>
> for (i = 0; i < num_entries; i++) {
> struct xe_pt *pt = entries[i].pt;
> @@ -996,7 +1018,7 @@ static void xe_pt_commit_prepare_bind(struct
> xe_vma *vma,
> if (xe_pt_entry(pt_dir, j_))
> oldpte = xe_pt_entry(pt_dir, j_);
>
> - pt_dir->children[j_] = &newpte->base;
> + pt_dir->staging[j_] = &newpte->base;
> entries[i].pt_entries[j].pt = oldpte;
> }
> }
> @@ -1237,7 +1259,10 @@ static void
> vma_convert_to_invalidation(struct xe_tile *tile, struct xe_vma
> *vma,
> struct xe_vm_pgtable_update_ops
> *pt_update)
> {
> - struct xe_pt_zap_ptes_flags flags = { .scratch = true, };
> + struct xe_pt_zap_ptes_flags flags = {
> + .scratch = true,
> + .staging = true,
> + };
> int i, j, k;
>
> /*
> @@ -1590,6 +1615,7 @@ static unsigned int xe_pt_stage_unbind(struct
> xe_tile *tile, struct xe_vma *vma,
> .ops = &xe_pt_stage_unbind_ops,
> .shifts = xe_normal_pt_shifts,
> .max_level = XE_PT_HIGHEST_LEVEL,
> + .staging = true,
> },
> .tile = tile,
> .modified_start = xe_vma_start(vma),
> @@ -1631,7 +1657,7 @@ static void xe_pt_abort_unbind(struct xe_vma
> *vma,
> {
> int i, j;
>
> - xe_pt_commit_locks_assert(vma);
> + xe_pt_commit_prepare_locks_assert(vma);
>
> for (i = num_entries - 1; i >= 0; --i) {
> struct xe_vm_pgtable_update *entry = &entries[i];
> @@ -1644,7 +1670,7 @@ static void xe_pt_abort_unbind(struct xe_vma
> *vma,
> continue;
>
> for (j = entry->ofs; j < entry->ofs + entry->qwords;
> j++)
> - pt_dir->children[j] =
> + pt_dir->staging[j] =
> entries[i].pt_entries[j - entry-
> >ofs].pt ?
> &entries[i].pt_entries[j - entry-
> >ofs].pt->base : NULL;
> }
> @@ -1657,7 +1683,7 @@ xe_pt_commit_prepare_unbind(struct xe_vma *vma,
> {
> int i, j;
>
> - xe_pt_commit_locks_assert(vma);
> + xe_pt_commit_prepare_locks_assert(vma);
>
> for (i = 0; i < num_entries; ++i) {
> struct xe_vm_pgtable_update *entry = &entries[i];
> @@ -1672,7 +1698,7 @@ xe_pt_commit_prepare_unbind(struct xe_vma *vma,
> for (j = entry->ofs; j < entry->ofs + entry->qwords;
> j++) {
> entry->pt_entries[j - entry->ofs].pt =
> xe_pt_entry(pt_dir, j);
> - pt_dir->children[j] = NULL;
> + pt_dir->staging[j] = NULL;
> }
> }
> }
> diff --git a/drivers/gpu/drm/xe/xe_pt_walk.c
> b/drivers/gpu/drm/xe/xe_pt_walk.c
> index b8b3d2aea492..be602a763ff3 100644
> --- a/drivers/gpu/drm/xe/xe_pt_walk.c
> +++ b/drivers/gpu/drm/xe/xe_pt_walk.c
> @@ -74,7 +74,8 @@ int xe_pt_walk_range(struct xe_ptw *parent,
> unsigned int level,
> u64 addr, u64 end, struct xe_pt_walk *walk)
> {
> pgoff_t offset = xe_pt_offset(addr, level, walk);
> - struct xe_ptw **entries = parent->children ? parent-
> >children : NULL;
> + struct xe_ptw **entries = walk->staging ? (parent->staging
> ?: NULL) :
> + (parent->children ?: NULL);
> const struct xe_pt_walk_ops *ops = walk->ops;
> enum page_walk_action action;
> struct xe_ptw *child;
> diff --git a/drivers/gpu/drm/xe/xe_pt_walk.h
> b/drivers/gpu/drm/xe/xe_pt_walk.h
> index 5ecc4d2f0f65..5c02c244f7de 100644
> --- a/drivers/gpu/drm/xe/xe_pt_walk.h
> +++ b/drivers/gpu/drm/xe/xe_pt_walk.h
> @@ -11,12 +11,14 @@
> /**
> * struct xe_ptw - base class for driver pagetable subclassing.
> * @children: Pointer to an array of children if any.
> + * @staging: Pointer to an array of staging if any.
> *
> * Drivers could subclass this, and if it's a page-directory,
> typically
> * embed an array of xe_ptw pointers.
> */
> struct xe_ptw {
> struct xe_ptw **children;
> + struct xe_ptw **staging;
> };
>
> /**
> @@ -41,6 +43,8 @@ struct xe_pt_walk {
> * as shared pagetables.
> */
> bool shared_pt_mode;
> + /** @staging: Walk staging PT structure */
> + bool staging;
> };
>
> /**
More information about the Intel-xe
mailing list