[Mesa-stable] [Mesa-dev] [PATCH 1/2] Fix races during _mesa_HashWalk().

[Timothy Arceri]

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 */