On Sun, 2016-11-06 at 12:16 +0100, Steinar H. Gunderson wrote:
> On Sun, Nov 06, 2016 at 09:35:37AM +1100, Timothy Arceri wrote:
> >
> > Can you please change this line to say something like. With this
> > change
> > we switch to using a recursive mutex, which avoids the deadlock.
>
> Rewording.
>
> >
> > I was a little confused. At first I thought it was already using a
> > recursive mutex.
>
> Well, I looked at the fallback (non-C11) implementations, and
> actually,
> they both are recursive no matter what you ask for. But on a C11
> compiler,
> it looks like it they won't be recursive, indeed.
>
> >
> > One thing that concerns me is that we can now call _mesa_HashRemove
> > from the callback in _mesa_HashDeleteAll.
> >
> > I think we may need a patch before this one that changes
> > _mesa_HashDeleteAll to wrap _mesa_hash_table_remove(table->ht,
> > entry);
> > with a deleted entry check otherwise the entry counts will get all
> > messed up.
> >
> > /* Check that the cb didn't already delete the entry. If not
> > * remove it.
> > */
> > if (entry->key != table->ht->deleted_key) {
> > _mesa_hash_table_remove(table->ht, entry);
> > }
>
> I'm not sure if I understand this part. Are you concerned that
> someone will
> be writing such code in the future?
Yes. Currently we get stuck waiting for a lock so no one will do it.
But after this fix it will be possible to call _mesa_HashRevmove from
withing the _mesa_HashDeleteAll cb which is entirely reasonable.
> Can't we just document _mesa_HashDeleteAll
> to say that this is illegal?
The problem is is can happen totally unintentionally. Anyway I've sent
a couple of patches as a follow-up to your series, one to fix it and
one to make use of it.
https://patchwork.freedesktop.org/series/14902/
Thanks for fixing these :)
>
> /* Steinar */
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
