Re: [v3] libstdc++/17780

[Paolo Carlini <pcarlini at suse dot de>]

Benjamin Kosnik wrote:

> I'd like to put this already big patch in, and then put in the tuning
> optimizations for it next. There are some obvious simplifications that
> can be done in _M_reserve block, where the operator new calls can be
> coalesced. That's later though.
>  
Nice!

Benjamin, there is a bit that worries me in the src/mt_allocator.cc changes:

    if (__gthread_active_p())
      {
-	__gthread_mutex_lock(__bin._M_mutex);
	if (__bin._M_first[0] == NULL)
	  {
-	    // No need to hold the lock when we are adding a
-	    // whole chunk to our own list.
-	    __gthread_mutex_unlock(__bin._M_mutex);


Basically, you don't take the lock before checking _bin._M_first[0] == NULL.

Some months ago, I considered this simplification of the original code, but
found it unsafe: if two different threads find __bin._M_first[0] != NULL
both proceed to the branch of the if that removes blocks from the global pool.
Indeed, only one at a time actually executes the code (there is a lock at the
beginning of the branch), but as soon as the first one is done, the other
one *also* wants to execute it and _bin._M_first[0] can now be NULL, because
the first thread may have removed all the blocks!!

What do you think??

Thanks,
Paolo.