[PATCH] Fix getpid/raise

[PATCH] Fix getpid/raise

[Jakub Jelinek]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

Hi!

The attached testcase fails with current CVS (when not linked against
-lpthread):

tgkill(32233, 32233, SIGUSR1)           = 0
--- SIGUSR1 (User defined signal 1) @ 0 (0) ---
sigreturn()                             = ? (mask now [])
tgkill(0, 32233, SIGUSR1)               = -1 EINVAL (Invalid argument)

Don't know where to put the tst-raise1.c testcase, signal/ or somewhere
else (nptl/ is bad, since it should not be linked against -lpthread)?

Additionally, I think there is a race.  If fork () is called when getpid ()
has never been called before in the program, then after the self->pid =
-parentpid; in fork.c but before the clone () syscall a signal is received
which calls getpid () and another signal is received in the child before
self->pid = self->tid setting, getpid () in that signal handler will return
incorrect value.

The attached patch should cure both.  Performance wise, I tried:
#include <stdio.h>
#include <dlfcn.h>

int main (void)
{
  int i;
  unsigned long long st, en, mi = ~0ULL;
  for (i = 0; i < 100; ++i)
    {
      asm volatile ("rdtsc" : "=A" (st));
      getpid ();
      asm volatile ("rdtsc" : "=A" (en));
      if (en - st < mi)
        mi = en - st;
    }
  printf ("%lld ticks\n", mi);
  dlopen ("libpthread.so.0", RTLD_LAZY);
  mi = ~0ULL;
  for (i = 0; i < 100; ++i)
    {
      asm volatile ("rdtsc" : "=A" (st));
      getpid ();
      asm volatile ("rdtsc" : "=A" (en));
      if (en - st < mi)
        mi = en - st;
    }
  printf ("%lld ticks\n", mi);
}
on my PIII, against the installed glibc which doesn't yet have any of the
getpid changes it prints
356 ticks
357 ticks

against CVS libc.so:
37 ticks
39 ticks

and against CVS+this patch:
41 ticks
38 ticks

I've moved pid field closer to tid, so that they share a cache line when
they are used so often together.

	Jakub

[-- Attachment #2: P1 --]
[-- Type: text/plain, Size: 3571 bytes --]

2003-12-23  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/unix/sysv/linux/raise.c (raise): Protect pid = selftid
	setting with __ASSUME_TGKILL || defined __NR_tgkill.
	If pid is 0, set it to selftid.
	* sysdeps/unix/sysv/linux/getpid.c (really_getpid): Make inline.
	Don't set self->pid but self->tid.  If self->pid == 0 and self->tid
	!= 0, return self->tid without doing a syscall.
	* descr.h (struct pthread): Move pid field after tid.

--- libc/nptl/sysdeps/unix/sysv/linux/raise.c.jj	2003-12-21 12:55:41.000000000 +0100
+++ libc/nptl/sysdeps/unix/sysv/linux/raise.c	2003-12-23 15:11:10.000000000 +0100
@@ -44,17 +44,19 @@ raise (sig)
 #endif
       THREAD_SETMEM (pd, tid, selftid);
 
+#if __ASSUME_TGKILL || defined __NR_tgkill
       /* We do not set the PID field in the TID here since we might be
 	 called from a signal handler while the thread executes fork.  */
       pid = selftid;
+#endif
     }
 #if __ASSUME_TGKILL || defined __NR_tgkill
   else
     /* raise is an async-safe function.  It could be called while the
        fork function temporarily invalidated the PID field.  Adjust for
        that.  */
-    if (__builtin_expect (pid < 0, 0))
-      pid = -pid;
+    if (__builtin_expect (pid <= 0, 0))
+      pid = pid == 0 ? selftid : -pid;
 #endif
 
 #if __ASSUME_TGKILL
--- libc/nptl/sysdeps/unix/sysv/linux/getpid.c.jj	2003-12-21 12:55:41.000000000 +0100
+++ libc/nptl/sysdeps/unix/sysv/linux/getpid.c	2003-12-23 16:07:57.000000000 +0100
@@ -23,32 +23,42 @@
 
 
 #ifndef NOT_IN_libc
-static pid_t really_getpid (pid_t oldval);
-#endif
-
+static inline __attribute__((always_inline)) pid_t really_getpid (pid_t oldval);
 
-pid_t
-__getpid (void)
+static inline __attribute__((always_inline)) pid_t
+really_getpid (pid_t oldval)
 {
-#ifndef NOT_IN_libc
-  pid_t result = THREAD_GETMEM (THREAD_SELF, pid);
-  if (__builtin_expect (result <= 0, 0))
-    result = really_getpid (result);
+  pid_t selftid;
+  if (__builtin_expect (oldval == 0
+			&& ((selftid = THREAD_GETMEM (THREAD_SELF, tid))
+			    != 0), 1))
+    return selftid;
+
+  INTERNAL_SYSCALL_DECL (err);
+  pid_t result = INTERNAL_SYSCALL (getpid, err, 0);
+
+  /* We do not set the PID field in the TID here since we might be
+     called from a signal handler while the thread executes fork.  */
+  if (oldval == 0)
+    THREAD_SETMEM (THREAD_SELF, tid, result);
   return result;
 }
+#endif
 
-static pid_t
-really_getpid (pid_t oldval)
+pid_t
+__getpid (void)
 {
-#endif
+#ifdef NOT_IN_libc
   INTERNAL_SYSCALL_DECL (err);
   pid_t result = INTERNAL_SYSCALL (getpid, err, 0);
-#ifndef NOT_IN_libc
-  if (oldval == 0)
-    THREAD_SETMEM (THREAD_SELF, pid, result);
+#else
+  pid_t result = THREAD_GETMEM (THREAD_SELF, pid);
+  if (__builtin_expect (result <= 0, 0))
+    result = really_getpid (result);
 #endif
   return result;
 }
+
 libc_hidden_def (__getpid)
 weak_alias (__getpid, getpid)
 libc_hidden_def (getpid)
--- libc/nptl/descr.h.jj	2003-07-23 01:04:00.000000000 +0200
+++ libc/nptl/descr.h	2003-12-23 16:17:55.000000000 +0100
@@ -121,6 +121,9 @@ struct pthread
      therefore stack) used' flag.  */
   pid_t tid;
 
+  /* Process ID - thread group ID in kernel speak.  */
+  pid_t pid;
+
   /* List of cleanup buffers.  */
   struct _pthread_cleanup_buffer *cleanup;
 
@@ -178,9 +181,6 @@ struct pthread
   /* Two-level array for the thread-specific data.  */
   struct pthread_key_data *specific[PTHREAD_KEY_1STLEVEL_SIZE];
 
-  /* Process ID - thread group ID in kernel speak.  */
-  pid_t pid;
-
   /* True if events must be reported.  */
   bool report_events;
 

[-- Attachment #3: tst-raise1.c --]
[-- Type: text/plain, Size: 667 bytes --]

#include <errno.h>
#include <error.h>
#include <signal.h>
#include <stdlib.h>

volatile int count;

void
sh (int sig)
{
  ++count;
}

int
main (void)
{
  struct sigaction sa;
  sa.sa_handler = sh;
  sigemptyset (&sa.sa_mask);
  sa.sa_flags = 0;
  if (sigaction (SIGUSR1, &sa, NULL) < 0)
    {
      printf ("sigaction failed: %m\n");
      exit (1);
    }
  if (raise (SIGUSR1) < 0)
    {
      printf ("first raise failed: %m\n");
      exit (1);
    }
  if (raise (SIGUSR1) < 0)
    {
      printf ("second raise failed: %m\n");
      exit (1);
    }
  if (count != 2)
    {
      printf ("signal handler not called 2 times\n");
      exit (1);
    }
  exit (0);
}

Re: [PATCH] Fix getpid/raise

[Roland McGrath]

> Don't know where to put the tst-raise1.c testcase, signal/ or somewhere
> else (nptl/ is bad, since it should not be linked against -lpthread)?

signal/ is the right place.

> The attached patch should cure both.  Performance wise, I tried:

Your code looks good to me.

Re: [PATCH] Fix getpid/raise

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Applied.

- -- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (GNU/Linux)

iD8DBQE/7Tab2ijCOnn/RHQRAj1MAJ9JT92ZtVodVyyvx1PYNEfkIEufMgCgx5hA
KeGZowHDBK+QmXUzeCc/TMU=
=rkdZ
-----END PGP SIGNATURE-----