On Fri, Sep 26, 2025 at 3:54 PM Ian Forbes <[email protected]> wrote: > > This hashtable is only used under a lock in vmw_execbuf_process and needs > to be cleared before vmw_execbuf_process returns otherwise bad things > happen because the nodes that are stored in the table come from an arena > allocator that is cleared at the end of the function. > > Rather than wasting time cleaning up the hashtable move it onto the stack > so we don't have to do any cleanup. > > Signed-off-by: Ian Forbes <[email protected]> > --- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 9 ----- > drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 3 -- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 3 -- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 43 +++------------------- > drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 5 ++- > 5 files changed, 9 insertions(+), 54 deletions(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > index 8ff958d119be..4b53d751b0e0 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c > @@ -814,13 +814,6 @@ static void vmw_write_driver_id(struct vmw_private *dev) > } > } > > -static void vmw_sw_context_init(struct vmw_private *dev_priv) > -{ > - struct vmw_sw_context *sw_context = &dev_priv->ctx; > - > - hash_init(sw_context->res_ht); > -} > - > static void vmw_sw_context_fini(struct vmw_private *dev_priv) > { > struct vmw_sw_context *sw_context = &dev_priv->ctx; > @@ -836,8 +829,6 @@ static int vmw_driver_load(struct vmw_private *dev_priv, > u32 pci_id) > enum vmw_res_type i; > bool refuse_dma = false; > > - vmw_sw_context_init(dev_priv); > - > mutex_init(&dev_priv->cmdbuf_mutex); > mutex_init(&dev_priv->binding_mutex); > spin_lock_init(&dev_priv->resource_lock); > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > index eda5b6f8f4c4..e8004a0a68c9 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h > @@ -78,7 +78,6 @@ > #define VMW_RES_STREAM ttm_driver_type2 > #define VMW_RES_FENCE ttm_driver_type3 > #define VMW_RES_SHADER ttm_driver_type4 > -#define VMW_RES_HT_ORDER 12 > > #define MKSSTAT_CAPACITY_LOG2 5U > #define MKSSTAT_CAPACITY (1U << MKSSTAT_CAPACITY_LOG2) > @@ -347,7 +346,6 @@ struct vmw_ctx_validation_info; > > /** > * struct vmw_sw_context - Command submission context > - * @res_ht: Pointer hash table used to find validation duplicates > * @kernel: Whether the command buffer originates from kernel code rather > * than from user-space > * @fp: If @kernel is false, points to the file of the client. Otherwise > @@ -377,7 +375,6 @@ struct vmw_ctx_validation_info; > * @ctx: The validation context > */ > struct vmw_sw_context{ > - DECLARE_HASHTABLE(res_ht, VMW_RES_HT_ORDER); > bool kernel; > struct vmw_fpriv *fp; > struct drm_file *filp; > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index 819704ac675d..6a7a9fc13a7f 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -4172,8 +4172,6 @@ int vmw_execbuf_process(struct drm_file *file_priv, > if (unlikely(ret != 0)) > goto out_err; > > - vmw_validation_drop_ht(&val_ctx); > - > ret = mutex_lock_interruptible(&dev_priv->binding_mutex); > if (unlikely(ret != 0)) { > ret = -ERESTARTSYS; > @@ -4282,7 +4280,6 @@ int vmw_execbuf_process(struct drm_file *file_priv, > __vmw_execbuf_release_pinned_bo(dev_priv, NULL); > out_unlock: > vmw_cmdbuf_res_revert(&sw_context->staged_cmd_res); > - vmw_validation_drop_ht(&val_ctx); > WARN_ON(!list_empty(&sw_context->ctx_list)); > mutex_unlock(&dev_priv->cmdbuf_mutex); > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > index 35dc94c3db39..1da7dbef905f 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.c > @@ -166,7 +166,7 @@ vmw_validation_find_bo_dup(struct vmw_validation_context > *ctx, > struct vmwgfx_hash_item *hash; > unsigned long key = (unsigned long) vbo; > > - hash_for_each_possible_rcu(ctx->sw_context->res_ht, hash, > head, key) { > + hash_for_each_possible(ctx->res_ht, hash, head, key) { > if (hash->key == key) { > bo_node = container_of(hash, > typeof(*bo_node), hash); > break; > @@ -208,7 +208,7 @@ vmw_validation_find_res_dup(struct vmw_validation_context > *ctx, > struct vmwgfx_hash_item *hash; > unsigned long key = (unsigned long) res; > > - hash_for_each_possible_rcu(ctx->sw_context->res_ht, hash, > head, key) { > + hash_for_each_possible(ctx->res_ht, hash, head, key) { > if (hash->key == key) { > res_node = container_of(hash, > typeof(*res_node), hash); > break; > @@ -258,8 +258,7 @@ int vmw_validation_add_bo(struct vmw_validation_context > *ctx, > > if (ctx->sw_context) { > bo_node->hash.key = (unsigned long) vbo; > - hash_add_rcu(ctx->sw_context->res_ht, > &bo_node->hash.head, > - bo_node->hash.key); > + hash_add(ctx->res_ht, &bo_node->hash.head, > bo_node->hash.key); > } > val_buf = &bo_node->base; > vmw_bo_reference(vbo); > @@ -305,11 +304,11 @@ int vmw_validation_add_resource(struct > vmw_validation_context *ctx, > > if (ctx->sw_context) { > node->hash.key = (unsigned long) res; > - hash_add_rcu(ctx->sw_context->res_ht, &node->hash.head, > node->hash.key); > + hash_add(ctx->res_ht, &node->hash.head, node->hash.key); > } > node->res = vmw_resource_reference_unless_doomed(res); > if (!node->res) { > - hash_del_rcu(&node->hash.head); > + hash_del(&node->hash.head); > return -ESRCH; > } > > @@ -611,38 +610,6 @@ int vmw_validation_res_validate(struct > vmw_validation_context *ctx, bool intr) > return 0; > } > > -/** > - * vmw_validation_drop_ht - Reset the hash table used for duplicate finding > - * and unregister it from this validation context. > - * @ctx: The validation context. > - * > - * The hash table used for duplicate finding is an expensive resource and > - * may be protected by mutexes that may cause deadlocks during resource > - * unreferencing if held. After resource- and buffer object registering, > - * there is no longer any use for this hash table, so allow freeing it > - * either to shorten any mutex locking time, or before resources- and > - * buffer objects are freed during validation context cleanup. > - */ > -void vmw_validation_drop_ht(struct vmw_validation_context *ctx) > -{ > - struct vmw_validation_bo_node *entry; > - struct vmw_validation_res_node *val; > - > - if (!ctx->sw_context) > - return; > - > - list_for_each_entry(entry, &ctx->bo_list, base.head) > - hash_del_rcu(&entry->hash.head); > - > - list_for_each_entry(val, &ctx->resource_list, head) > - hash_del_rcu(&val->hash.head); > - > - list_for_each_entry(val, &ctx->resource_ctx_list, head) > - hash_del_rcu(&val->hash.head); > - > - ctx->sw_context = NULL; > -} > - > /** > * vmw_validation_unref_lists - Unregister previously registered buffer > * object and resources. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h > b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h > index 353d837907d8..2b82a1a3110d 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_validation.h > @@ -37,10 +37,11 @@ > #define VMW_RES_DIRTY_NONE 0 > #define VMW_RES_DIRTY_SET BIT(0) > #define VMW_RES_DIRTY_CLEAR BIT(1) > +#define VMW_RES_HT_ORDER 7 > > /** > * struct vmw_validation_context - Per command submission validation context > - * @ht: Hash table used to find resource- or buffer object duplicates > + * @res_ht: Hash table used to find resource- or buffer object duplicates > * @resource_list: List head for resource validation metadata > * @resource_ctx_list: List head for resource validation metadata for > * resources that need to be validated before those in @resource_list > @@ -55,6 +56,7 @@ > */ > struct vmw_validation_context { > struct vmw_sw_context *sw_context; > + DECLARE_HASHTABLE(res_ht, VMW_RES_HT_ORDER); > struct list_head resource_list; > struct list_head resource_ctx_list; > struct list_head bo_list; > @@ -84,6 +86,7 @@ struct vmw_fence_obj; > #define DECLARE_VAL_CONTEXT(_name, _sw_context, _merge_dups) \ > struct vmw_validation_context _name = \ > { .sw_context = _sw_context, \ > + .res_ht = {}, \ > .resource_list = LIST_HEAD_INIT((_name).resource_list), \ > .resource_ctx_list = LIST_HEAD_INIT((_name).resource_ctx_list), \ > .bo_list = LIST_HEAD_INIT((_name).bo_list), \ > -- > 2.51.0 >
Could you make this one part of a seperate series i.e. "Cleanup resource hashtable usage" or such. As is it's a bit of half-way done, e.g. DECLARE_VAL_CONTEXT still takes sw_context, for which the only reason was the hashtable which is not part of it anymore. I think we'd want to finish this cleanup, get rid of sw_context usage in validation and remove it from DECLARE_VAL_CONTEXT. This also gives us a more natural spot to land those patch (the first two in drm-misc-fixes and this series in drm-misc-next). z z z
smime.p7s
Description: S/MIME Cryptographic Signature
