Patchwork [libstdc++] Improve slightly __cxa_guard_acquire

login
register
mail settings
Submitter Thiago Macieira
Date Aug. 30, 2012, 10:48 a.m.
Message ID <2255222.Ergk1VO0fz@tjmaciei-mobl2>
Download mbox | patch
Permalink /patch/180816/
State New
Headers show

Comments

Thiago Macieira - Aug. 30, 2012, 10:48 a.m.
Hello

The attached patch is a simple improvement to make a thread that failed to set 
the waiting bit to exit the function earlier, if it detects that another 
thread has successfully finished initialising. It matches the CAS code from a 
few lines above.

The change from RELAXED to ACQUIRE is noted in the previous patch I've just 
sent.
Benjamin Kosnik - Sept. 6, 2012, 6:45 p.m.
On Thu, 30 Aug 2012 12:48:34 +0200
Thiago Macieira <thiago.macieira@intel.com> wrote:

> Hello
> 
> The attached patch is a simple improvement to make a thread that
> failed to set the waiting bit to exit the function earlier, if it
> detects that another thread has successfully finished initialising.
> It matches the CAS code from a few lines above.
> 
> The change from RELAXED to ACQUIRE is noted in the previous patch
> I've just sent.

I like this, but want

// make a thread that failed to set the waiting bit to exit the function
// earlier, if it detects that another thread has successfully finished
// initialising

added as a comment in the

if (expected == pending_bit)

branch. 

I would like to put this in trunk + comment and give it 2-3 days at
least before 4.7 branch.

-benjamin
Jakub Jelinek - Sept. 6, 2012, 9:10 p.m.
On Thu, Sep 06, 2012 at 01:33:11PM -0700, Benjamin De Kosnik wrote:
> Here's the patch as applied to trunk in rev. 191042. I'll apply it to
> 4.7 this weekend as long as nobody yelps.

> 2012-09-06  Thiago Macieira  <thiago.macieira@intel.com>
> 
> 	PR libstdc++/54172
>         * libsupc++/guard.cc (__cxa_guard_acquire): Exit the loop earlier if
>         we detect that another thread has had success. Don't compare_exchange
>         from a finished state back to a waiting state. Comment.
> 
> diff --git a/libstdc++-v3/libsupc++/guard.cc b/libstdc++-v3/libsupc++/guard.cc
> index adc9608..60165cd 100644
> --- a/libstdc++-v3/libsupc++/guard.cc
> +++ b/libstdc++-v3/libsupc++/guard.cc
> @@ -244,13 +244,13 @@ namespace __cxxabiv1
>      if (__gthread_active_p ())
>        {
>  	int *gi = (int *) (void *) g;
> -	int expected(0);
>  	const int guard_bit = _GLIBCXX_GUARD_BIT;
>  	const int pending_bit = _GLIBCXX_GUARD_PENDING_BIT;
>  	const int waiting_bit = _GLIBCXX_GUARD_WAITING_BIT;
>  
>  	while (1)
>  	  {
> +	    int expected(0);
>  	    if (__atomic_compare_exchange_n(gi, &expected, pending_bit, false,
>  					    __ATOMIC_ACQ_REL,
>  					    __ATOMIC_RELAXED))

Shouldn't this __ATOMIC_RELAXED be also __ATOMIC_ACQUIRE?  If expected ends
up being guard_bit, then the code will return 0; right away.

> @@ -264,13 +264,26 @@ namespace __cxxabiv1
>  		// Already initialized.
>  		return 0;	
>  	      }
> +
>  	     if (expected == pending_bit)
>  	       {
> +		 // Use acquire here.
>  		 int newv = expected | waiting_bit;
>  		 if (!__atomic_compare_exchange_n(gi, &expected, newv, false,
>  						  __ATOMIC_ACQ_REL, 
> -						  __ATOMIC_RELAXED))
> -		   continue;
> +						  __ATOMIC_ACQUIRE))
> +		   {
> +		     if (expected == guard_bit)
> +		       {
> +			 // Make a thread that failed to set the
> +			 // waiting bit exit the function earlier,
> +			 // if it detects that another thread has
> +			 // successfully finished initialising.
> +			 return 0;
> +		       }
> +		     if (expected == 0)
> +		       continue;
> +		   }
>  		 
>  		 expected = newv;
>  	       }


	Jakub

Patch

2012-08-30  Thiago Macieira <thiago.macieira@intel.com>

	* libsupc++/guard.cc (__cxa_guard_acquire): exit the loop earlier if
	we detect that another thread has had success.
---
 libstdc++-v3/libsupc++/guard.cc | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/libstdc++-v3/libsupc++/guard.cc b/libstdc++-v3/libsupc++/guard.cc
index bfe1a59..73d7221 100644
--- a/libstdc++-v3/libsupc++/guard.cc
+++ b/libstdc++-v3/libsupc++/guard.cc
@@ -269,8 +269,16 @@  namespace __cxxabiv1
 		 int newv = expected | waiting_bit;
 		 if (!__atomic_compare_exchange_n(gi, &expected, newv, false,
 						  __ATOMIC_ACQ_REL, 
-						  __ATOMIC_RELAXED))
-		   continue;
+						  __ATOMIC_ACQUIRE))
+		   {
+		     if (expected == guard_bit)
+		       {
+			 // Already initialized.
+			 return 0;
+		       }
+		     if (expected == 0)
+		       continue;
+		   }
 		 
 		 expected = newv;
 	       }
-- 
1.7.11.4