[Bug nptl/33704] RFE: Relax failing pthread_mutex_trylock to load with relaxed MO.

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=33704

--- Comment #11 from Florian Weimer <fweimer at redhat dot com> ---
My comment 10 is not correct, I will get to that in a minute. But here's what I
did to get a feel  for how these functions are used.

I applied this patch:

diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c
index 94621cc254..6ef9e9bedf 100644
--- a/nptl/pthread_mutex_trylock.c
+++ b/nptl/pthread_mutex_trylock.c
@@ -22,6 +22,8 @@
 #include <lowlevellock.h>
 #include <futex-internal.h>

+#include <stap-probe.h>
+
 int
 ___pthread_mutex_trylock (pthread_mutex_t *mutex)
 {
@@ -44,6 +46,7 @@ ___pthread_mutex_trylock (pthread_mutex_t *mutex)
            return EAGAIN;

          ++mutex->__data.__count;
+         LIBC_PROBE (mutex_trylock_recursive_recurse, 1, mutex);
          return 0;
        }

@@ -53,20 +56,26 @@ ___pthread_mutex_trylock (pthread_mutex_t *mutex)
          mutex->__data.__owner = id;
          mutex->__data.__count = 1;
          ++mutex->__data.__nusers;
+         LIBC_PROBE (mutex_trylock_recursive_success, 1, mutex);
          return 0;
        }
+      LIBC_PROBE (mutex_trylock_recursive_fail, 1, mutex);
       break;

     case PTHREAD_MUTEX_TIMED_NP:
     case PTHREAD_MUTEX_ADAPTIVE_NP:
     case PTHREAD_MUTEX_ERRORCHECK_NP:
       if (lll_trylock (mutex->__data.__lock) != 0)
-       break;
+       {
+         LIBC_PROBE (mutex_trylock_fail, 1, mutex);
+         break;
+       }

       /* Record the ownership.  */
       mutex->__data.__owner = id;
       ++mutex->__data.__nusers;

+      LIBC_PROBE (mutex_trylock_success, 1, mutex);
       return 0;

     case PTHREAD_MUTEX_ROBUST_RECURSIVE_NP:

Installed this glibc system-wide, and rebooted for good measure. Then run this
stap probe:

“
global success
global fail
global rec_success
global rec_recurse
global rec_fail

probe process("/lib64/libc.so.6").mark("mutex_trylock_success") {
    success <<< 1
}

probe process("/lib64/libc.so.6").mark("mutex_trylock_fail") {
    fail <<< 1
}

probe process("/lib64/libc.so.6").mark("mutex_trylock_recursive_recurse") {
    rec_recurse <<< 1
}

probe process("/lib64/libc.so.6").mark("mutex_trylock_recursive_success") {
    rec_success <<< 1
}

probe process("/lib64/libc.so.6").mark("mutex_trylock_recursive_fail") {
    rec_fail <<< 1
}

probe timer.s(1) {
    printf("success=%d fail=%d recurse=%d rec_success=%d rec_fail=%d\n",
      @count(success),
      @count(fail),
      @count(rec_recurse),
      @count(rec_success),
      @count(rec_fail))
    delete success
    delete fail
    delete rec_recurse
    delete rec_success
    delete rec_fail
}
“

My regular desktop does not show much of trylock activity. But running Gmail in
Chromium results in this:

success=223549 fail=919 recurse=0 rec_success=0 rec_fail=0
success=39439 fail=109 recurse=0 rec_success=1 rec_fail=0
success=61005 fail=179 recurse=0 rec_success=0 rec_fail=0
success=161705 fail=566 recurse=0 rec_success=0 rec_fail=0
success=284841 fail=1779 recurse=0 rec_success=1 rec_fail=0

After a while the background activity drops to this:

success=365 fail=0 recurse=0 rec_success=0 rec_fail=0
success=187 fail=0 recurse=0 rec_success=0 rec_fail=0
success=207 fail=0 recurse=0 rec_success=0 rec_fail=0
success=5983 fail=9 recurse=0 rec_success=0 rec_fail=0
success=1596 fail=2 recurse=0 rec_success=0 rec_fail=0
success=9831 fail=8 recurse=0 rec_success=0 rec_fail=0
success=530 fail=0 recurse=0 rec_success=0 rec_fail=0

Anyway, my point is that a succeeding trylock in common workloads is much, much
more common than a failing trylock, so it makes sense to optimize for a
successful trylock.

However, I'm still wrong because we first read the type of the mutex. So this
is likely to bring in the cache line in a shared configuration anyway. After
that, the extra check on the lock field before the CAS should be basically
free. So your patch doesn't seem unreasonable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.