[PATCH] [libstdc++] Fix "bare" notifications dropped by waiters check

[PATCH] [libstdc++] Fix "bare" notifications dropped by waiters check

[Thomas Rodgers]

From: Thomas Rodgers <rodgert@twrodgers.com>

NOTE - This patch also needs to be backported to gcc-11 in order for
semaphore release() to work correctly on non-futex platforms.

Tested sparc-sun-solaris2.11

For types that track whether or not there extant waiters (e.g.
semaphore) internally, the __atomic_notify_address_bare() call was
introduced to avoid the overhead of loading the atomic count of
waiters. For platforms that don't have Futex, however, there was
still a check for waiters, and seeing that there are none (because
in the bare case, the count is not incremented), the notification
is dropped. This commit addresses that case.

libstdc++-v3/ChangeLog:
	* include/bits/atomic_wait.h: Always notify waiters in the
	in the case of 'bare' address notification.
---
 libstdc++-v3/include/bits/atomic_wait.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
index 0ac5575190c..984ed70f16c 100644
--- a/libstdc++-v3/include/bits/atomic_wait.h
+++ b/libstdc++-v3/include/bits/atomic_wait.h
@@ -226,9 +226,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       }
 
       void
-      _M_notify(const __platform_wait_t* __addr, bool __all) noexcept
+      _M_notify(const __platform_wait_t* __addr, bool __all, bool __bare) noexcept
       {
-	if (!_M_waiting())
+	if (!(__bare || _M_waiting()))
 	  return;
 
 #ifdef _GLIBCXX_HAVE_PLATFORM_WAIT
@@ -304,11 +304,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	  }
 
 	void
-	_M_notify(bool __all)
+	_M_notify(bool __all, bool __bare = false)
 	{
 	  if (_M_addr == &_M_w._M_ver)
 	    __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
-	  _M_w._M_notify(_M_addr, __all);
+	  _M_w._M_notify(_M_addr, __all, __bare);
 	}
 
 	template<typename _Up, typename _ValFn,
@@ -452,7 +452,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __atomic_notify_address(const _Tp* __addr, bool __all) noexcept
     {
       __detail::__bare_wait __w(__addr);
-      __w._M_notify(__all);
+      __w._M_notify(__all, true);
     }
 
   // This call is to be used by atomic types which track contention externally
@@ -464,7 +464,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     __detail::__platform_notify(__addr, __all);
 #else
     __detail::__bare_wait __w(__addr);
-    __w._M_notify(__all);
+    __w._M_notify(__all, true);
 #endif
   }
 _GLIBCXX_END_NAMESPACE_VERSION
-- 
2.30.2

Re: [PATCH] [libstdc++] Fix "bare" notifications dropped by waiters check

[Richard Biener]

On Thu, Apr 22, 2021 at 4:29 AM Thomas Rodgers
<rodgert@appliantology.com> wrote:
>
> From: Thomas Rodgers <rodgert@twrodgers.com>
>
> NOTE - This patch also needs to be backported to gcc-11 in order for
> semaphore release() to work correctly on non-futex platforms.
>
> Tested sparc-sun-solaris2.11
>
> For types that track whether or not there extant waiters (e.g.
> semaphore) internally, the __atomic_notify_address_bare() call was
> introduced to avoid the overhead of loading the atomic count of
> waiters. For platforms that don't have Futex, however, there was
> still a check for waiters, and seeing that there are none (because
> in the bare case, the count is not incremented), the notification
> is dropped. This commit addresses that case.

This is OK for the GCC 11 branch if approved for trunk.

Richard.

> libstdc++-v3/ChangeLog:
>         * include/bits/atomic_wait.h: Always notify waiters in the
>         in the case of 'bare' address notification.
> ---
>  libstdc++-v3/include/bits/atomic_wait.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
> index 0ac5575190c..984ed70f16c 100644
> --- a/libstdc++-v3/include/bits/atomic_wait.h
> +++ b/libstdc++-v3/include/bits/atomic_wait.h
> @@ -226,9 +226,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>        }
>
>        void
> -      _M_notify(const __platform_wait_t* __addr, bool __all) noexcept
> +      _M_notify(const __platform_wait_t* __addr, bool __all, bool __bare) noexcept
>        {
> -       if (!_M_waiting())
> +       if (!(__bare || _M_waiting()))
>           return;
>
>  #ifdef _GLIBCXX_HAVE_PLATFORM_WAIT
> @@ -304,11 +304,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>           }
>
>         void
> -       _M_notify(bool __all)
> +       _M_notify(bool __all, bool __bare = false)
>         {
>           if (_M_addr == &_M_w._M_ver)
>             __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
> -         _M_w._M_notify(_M_addr, __all);
> +         _M_w._M_notify(_M_addr, __all, __bare);
>         }
>
>         template<typename _Up, typename _ValFn,
> @@ -452,7 +452,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      __atomic_notify_address(const _Tp* __addr, bool __all) noexcept
>      {
>        __detail::__bare_wait __w(__addr);
> -      __w._M_notify(__all);
> +      __w._M_notify(__all, true);
>      }
>
>    // This call is to be used by atomic types which track contention externally
> @@ -464,7 +464,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>      __detail::__platform_notify(__addr, __all);
>  #else
>      __detail::__bare_wait __w(__addr);
> -    __w._M_notify(__all);
> +    __w._M_notify(__all, true);
>  #endif
>    }
>  _GLIBCXX_END_NAMESPACE_VERSION
> --
> 2.30.2
>

Re: [PATCH] [libstdc++] Fix "bare" notifications dropped by waiters check

[Jonathan Wakely]

On 21/04/21 18:29 -0700, Thomas Rodgers wrote:
>From: Thomas Rodgers <rodgert@twrodgers.com>
>
>NOTE - This patch also needs to be backported to gcc-11 in order for
>semaphore release() to work correctly on non-futex platforms.
>
>Tested sparc-sun-solaris2.11
>
>For types that track whether or not there extant waiters (e.g.
>semaphore) internally, the __atomic_notify_address_bare() call was
>introduced to avoid the overhead of loading the atomic count of
>waiters. For platforms that don't have Futex, however, there was
>still a check for waiters, and seeing that there are none (because
>in the bare case, the count is not incremented), the notification
>is dropped. This commit addresses that case.
>
>libstdc++-v3/ChangeLog:
>	* include/bits/atomic_wait.h: Always notify waiters in the
>	in the case of 'bare' address notification.

Repeated text: "in the in the"


>---
> libstdc++-v3/include/bits/atomic_wait.h | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
>index 0ac5575190c..984ed70f16c 100644
>--- a/libstdc++-v3/include/bits/atomic_wait.h
>+++ b/libstdc++-v3/include/bits/atomic_wait.h
>@@ -226,9 +226,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       }
>
>       void
>-      _M_notify(const __platform_wait_t* __addr, bool __all) noexcept
>+      _M_notify(const __platform_wait_t* __addr, bool __all, bool __bare) noexcept
>       {
>-	if (!_M_waiting())
>+	if (!(__bare || _M_waiting()))

Maybe it's just me, but I would find (!__base && !__waiting) to be a
clearer expression of the logic here.

i.e. "return if the wait is not bare and there are no waiters"
rather than "return if the wait is not either bare or has waiters".
The latter makes me take a second to grok.

The patch is OK either way though, with the ChangeLog typo fix.


> 	  return;
>
> #ifdef _GLIBCXX_HAVE_PLATFORM_WAIT
>@@ -304,11 +304,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	  }
>
> 	void
>-	_M_notify(bool __all)
>+	_M_notify(bool __all, bool __bare = false)
> 	{
> 	  if (_M_addr == &_M_w._M_ver)
> 	    __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
>-	  _M_w._M_notify(_M_addr, __all);
>+	  _M_w._M_notify(_M_addr, __all, __bare);
> 	}
>
> 	template<typename _Up, typename _ValFn,
>@@ -452,7 +452,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     __atomic_notify_address(const _Tp* __addr, bool __all) noexcept
>     {
>       __detail::__bare_wait __w(__addr);
>-      __w._M_notify(__all);
>+      __w._M_notify(__all, true);
>     }
>
>   // This call is to be used by atomic types which track contention externally
>@@ -464,7 +464,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     __detail::__platform_notify(__addr, __all);
> #else
>     __detail::__bare_wait __w(__addr);
>-    __w._M_notify(__all);
>+    __w._M_notify(__all, true);
> #endif
>   }
> _GLIBCXX_END_NAMESPACE_VERSION
>-- 
>2.30.2
>

Re: [PATCH] [libstdc++] Fix "bare" notifications dropped by waiters check

[Thomas Rodgers]

On 2021-04-22 02:23, Jonathan Wakely wrote:

> On 21/04/21 18:29 -0700, Thomas Rodgers wrote:
> 
>> From: Thomas Rodgers <rodgert@twrodgers.com>
>> 
>> NOTE - This patch also needs to be backported to gcc-11 in order for
>> semaphore release() to work correctly on non-futex platforms.
>> 
>> Tested sparc-sun-solaris2.11
>> 
>> For types that track whether or not there extant waiters (e.g.
>> semaphore) internally, the __atomic_notify_address_bare() call was
>> introduced to avoid the overhead of loading the atomic count of
>> waiters. For platforms that don't have Futex, however, there was
>> still a check for waiters, and seeing that there are none (because
>> in the bare case, the count is not incremented), the notification
>> is dropped. This commit addresses that case.
>> 
>> libstdc++-v3/ChangeLog:
>> * include/bits/atomic_wait.h: Always notify waiters in the
>> in the case of 'bare' address notification.
> 
> Repeated text: "in the in the"
> 
>> ---
>> libstdc++-v3/include/bits/atomic_wait.h | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/libstdc++-v3/include/bits/atomic_wait.h 
>> b/libstdc++-v3/include/bits/atomic_wait.h
>> index 0ac5575190c..984ed70f16c 100644
>> --- a/libstdc++-v3/include/bits/atomic_wait.h
>> +++ b/libstdc++-v3/include/bits/atomic_wait.h
>> @@ -226,9 +226,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> }
>> 
>> void
>> -      _M_notify(const __platform_wait_t* __addr, bool __all) noexcept
>> +      _M_notify(const __platform_wait_t* __addr, bool __all, bool 
>> __bare) noexcept
>> {
>> -    if (!_M_waiting())
>> +    if (!(__bare || _M_waiting()))
> 
> Maybe it's just me, but I would find (!__base && !__waiting) to be a
> clearer expression of the logic here.
> 
> i.e. "return if the wait is not bare and there are no waiters"
> rather than "return if the wait is not either bare or has waiters".
> The latter makes me take a second to grok.
> 

As discussed on IRC, I went back and forth on this a couple of times and 
neither option seemed particularly great to me. I started down the path 
of propagating the std::true_type/std::false_type bits that the RAII 
wrapper type knows about and making this a compile time choice, but felt 
the change was too big to make this late in the release cycle. I will 
revisit this for stage1 (though ideally all of this will be moved into 
the .so for GCC12 and partially rewritten as part of that process 
anyway).

> The patch is OK either way though, with the ChangeLog typo fix.
> 
>> return;
>> 
>> #ifdef _GLIBCXX_HAVE_PLATFORM_WAIT
>> @@ -304,11 +304,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> }
>> 
>> void
>> -    _M_notify(bool __all)
>> +    _M_notify(bool __all, bool __bare = false)
>> {
>> if (_M_addr == &_M_w._M_ver)
>> __atomic_fetch_add(_M_addr, 1, __ATOMIC_ACQ_REL);
>> -      _M_w._M_notify(_M_addr, __all);
>> +      _M_w._M_notify(_M_addr, __all, __bare);
>> }
>> 
>> template<typename _Up, typename _ValFn,
>> @@ -452,7 +452,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> __atomic_notify_address(const _Tp* __addr, bool __all) noexcept
>> {
>> __detail::__bare_wait __w(__addr);
>> -      __w._M_notify(__all);
>> +      __w._M_notify(__all, true);
>> }
>> 
>> // This call is to be used by atomic types which track contention 
>> externally
>> @@ -464,7 +464,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> __detail::__platform_notify(__addr, __all);
>> #else
>> __detail::__bare_wait __w(__addr);
>> -    __w._M_notify(__all);
>> +    __w._M_notify(__all, true);
>> #endif
>> }
>> _GLIBCXX_END_NAMESPACE_VERSION
>> -- 2.30.2

Tested sparc-sun-solaris2.11.
Committed to master, backported to release/gcc-11.