On Wed, 2022-10-19 at 12:21 +0200, Thomas Hellström (Intel) wrote:
> ⚠ External Email
>
> On 10/17/22 21:54, Zack Rusin wrote:
> > From: Maaz Mombasawala <mombasawalam@xxxxxxxxxx>
> >
> > Vmwgfx's hashtab implementation needs to be replaced with linux/hashtable
> > to reduce maintenence burden.
> > As part of this effort, refactor the res_ht hashtable used for resource
> > validation during execbuf execution to use linux/hashtable implementation.
> > This also refactors vmw_validation_context to use vmw_sw_context as the
> > container for the hashtable, whereas before it used a vmwgfx_open_hash
> > directly. This makes vmw_validation_context less generic, but there is
> > no functional change since res_ht is the only instance where validation
> > context used a hashtable in vmwgfx driver.
>
> Why is a pointer to the vmw_sw_context added to the validation context,
> rather than a pointer to the new hash table type itself? Seems like an
> unnecessary indirection which adds a dependency on the sw context to the
> validation context?
Hi, Thomas. Thanks for taking a look at this! That's because the entire public
interface of hashtable depends on it being a struct on which sizeof works rather
than a pointer, i.e. almost the entire interface of hasthbale.h is defined and they
all require that HASH_SIZE(hashtable) which is defined as
#define HASH_SIZE(hashtable) (ARRAY_SIZE(hashtable))
works on the hashtable. So the interface of hashtable.h can't operate on pointers,
but rather needs the full struct.
So the only two options are either rewriting the functions from linux/hashtable.h to
take explicit HASH_SIZE(hashtable) or make sure that in the functions where you will
use hashtable.h you pass the parent struct so that sizeof on the hashtable works
correctly. I've seen both approaches in the kernel, but in this case we thought the
latter made more sense.
> > + if (ctx->sw_context) {
> > struct vmwgfx_hash_item *hash;
> > + unsigned long key = (unsigned long) vbo;
> >
> > - if (!vmwgfx_ht_find_item(ctx->ht, (unsigned long) vbo, &hash))
> > - bo_node = container_of(hash, typeof(*bo_node), hash);
> > + hash_for_each_possible_rcu(ctx->sw_context->res_ht, hash, head, key) {
>
> This open-coded loop seems to be used throughout the conversion, and
> also in other patches. Perhaps a small helper in place?
Yes, that's a good idea. We'll see if we can abstract that. The tricky bit of
abstracting code that's using hash_* functions is that, as mentioned above, these
are all macros that depend on hashtable not being a pointer, so if you can't fit
your helper in another define then you have the same choice as above (i.e. rewrite
whatever hash_* function you were using to allow for explicit hash size, or make
your helper take the parent of the hash as argument).
> Otherwise LGTM.
Thanks!
z
