Remove cached PID/TID in clone

  Hi Florian, thanks for the review.

On 07/11/2016 15:21, Florian Weimer wrote:
> On 10/13/2016 09:45 PM, Adhemerval Zanella wrote:
>> This patch remove the PID cache and usage in current GLIBC code.  Current
>> usage is mainly used for performance optimization to avoid the syscall,
>> however it adds some issues:
>>
>>   - The exposed clone syscall will try to set pid/tid to make the new
>>     thread somewhat compatible with current GLIBC assumptions.  This cause
>>     a set of issue with new workloads and usercases (such as BZ#17214 and
> 
> “usecases”

Ack, I changed it on this new version.

> 
>>     [1]) as well for new internal usage of clone to optimize other algorithms
>>     (such as clone plus CLONE_VM for posix_spawn, BZ#19957).
>>
>>   - The caching complexity also added some bugs in the past [2] [3] and
>>     requires more effort of each port to handle such requirements (for
>>     both clone and vfork implementation).
>>
>>   - Caching performance gain in mainly or getpid and some specific
>>     code paths. The getpid performance leverage is questionable [4],
>>     either by the idea of getpid being a hotspot as for the getpid
>>     implementation itself (if it is indeed a justifiable hotspot a
>>     vDSO symbol could let to a much more simpler solution).
> 
> It's a hotspot for incorrect/broken fork detection.

If you mean the assert on fork.c, I review the code and it seems
unnecessary to remove the assert on child creation:

146   if (pid == 0)
147     {
148       struct pthread *self = THREAD_SELF;
149 
150       assert (THREAD_GETMEM (self, tid) != ppid);
151 

I added it back. 

> 
>>     Other usage is mainly for non usual code paths, such as pthread
>>     cancellation signal and handling.
>>
>> For thread creation (on atack allocation) the code simplification in fact
> 
> “stack allocation”

Ack.

> 
>> adds some performance gain due the no need of transverse the stack
>> cache and invalidate each element pid.
>>
>> Other thread usages will require a direct getpid syscall, such as
>> cancellation/setxid signal, thread cancellation, thread fail path
>> (at create_thread), and thread signal (pthread_kill and
>> pthread_sigqueue).  However these are hardly usual hotspots and I
>> think adding a syscall is justifiable.
>>
>> It also simplifies both the clone and vfork arch-specific implementation.
>> And by review each fork implementation there are some discrepancies that
>> this patch also solves:
>>
>>   - microblaze clone/vfork does not set/reset the pid/tid field
>>   - hppa uses the default vfork implementation that fallback to fork.
>>     Since vfork is deprecated I do not think we should bother with it.
>>
>> The patch also removes the TID caching in clone. My understanding for
>> such semantic is try provide some pthread usage after a user program
>> issue clone directly (as done by thread creation with CLONE_PARENT_SETTID
>> and pthread tid member). However, as stated before in multiple threads,
>> GLIBC provides clone syscalls without futher supporting all this
> 
> “further”

Ack.

> 
>> semantics. It means that, although GLIBC currently tries a better effort,
>> since it does not make any more guarantees, specially for newer and newer
>> clone flags.
> 
> So the question is whether this is used internally.  Why do you think this is safe?  Because we set it again with SET_TID_ADDRESS?
> 

The tid fields is basically used internally on pthread implementations
(including getpid) and since correct usage means thread *must* be
created using pthread_create we are sure the tid field will be
correctly set due 'set_tid_address' from __pthread_initialize_pids.

>> diff --git a/nptl/descr.h b/nptl/descr.h
>> index 8e4938d..17a2c9f 100644
>> --- a/nptl/descr.h
>> +++ b/nptl/descr.h
>> @@ -167,7 +167,7 @@ struct pthread
>>       therefore stack) used' flag.  */
>>    pid_t tid;
>>
>> -  /* Process ID - thread group ID in kernel speak.  */
>> +  /* Ununsed.  */
>>    pid_t pid;
> 
> Please rename to “pid_unused” or something like that, to make sure it's no longer referenced.

I renamed it on my local branch and I also updated the change spot
that it incur:

> 
>> diff --git a/sysdeps/unix/sysv/linux/getpid.c b/sysdeps/unix/sysv/linux/getpid.c
>> index 1124549..2bfafed 100644
>> --- a/sysdeps/unix/sysv/linux/getpid.c
>> +++ b/sysdeps/unix/sysv/linux/getpid.c
> 
> Can you drop this file completely, so that the default implementation is used?

I do not have a preference here, but I think now we can use syscalls.list
instead.  I change it on this version.

> 
>> diff --git a/sysdeps/unix/sysv/linux/pthread_kill.c b/sysdeps/unix/sysv/linux/pthread_kill.c
> 
>> @@ -49,14 +50,15 @@ __pthread_kill (pthread_t threadid, int signo)
>>    /* We have a special syscall to do the work.  */
>>    INTERNAL_SYSCALL_DECL (err);
>>
>> +  pid_t pid = getpid ();
> 
> Use __getpid for consistency?

Alright, I change it.

> 
>>    /* One comment: The PID field in the TCB can temporarily be changed
>>       (in fork).  But this must not affect this code here.  Since this
>>       function would have to be called while the thread is executing
>>       fork, it would have to happen in a signal handler.  But this is
>>       no allowed, pthread_kill is not guaranteed to be async-safe.  */
> 
> Comment is outdated.

Ack, I removed this implementation.

> 
>> diff --git a/sysdeps/unix/sysv/linux/pthread_sigqueue.c b/sysdeps/unix/sysv/linux/pthread_sigqueue.c
>> index 7694d54..642366b 100644
>> --- a/sysdeps/unix/sysv/linux/pthread_sigqueue.c
>> +++ b/sysdeps/unix/sysv/linux/pthread_sigqueue.c
>> @@ -49,12 +49,14 @@ pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
>>    if (signo == SIGCANCEL || signo == SIGTIMER || signo == SIGSETXID)
>>      return EINVAL;
>>
>> +  pid_t pid = getpid ();
> 
> Use __getpid for consistency?

Ack.

> 
>>      function would have to be called while the thread is executing
>>      fork, it would have to happen in a signal handler.  But this is
> 
> Comment is outdated.

Ack, I removed it.
> 
>> diff --git a/sysdeps/unix/sysv/linux/tst-clone2.c b/sysdeps/unix/sysv/linux/tst-clone2.c
>> index 68a7e6d..b20332a 100644
>> --- a/sysdeps/unix/sysv/linux/tst-clone2.c
> 
> It may make sense to update the file comment.
> 
>> -  pid_t pid = THREAD_GETMEM (THREAD_SELF, pid);
>> -  pid_t tid = THREAD_GETMEM (THREAD_SELF, tid);
>> +  /* Clone without flags do not cache the pid and tid is only set in thread
> 
> “does not cache”.  But the comment seems outdated?

Maybe, my initial idea was to make sure that of this caching
not being used anymore.  I removed this comment as well.

> 
>> +     creation by using CLONE_PARENT_SETTID plus pthread tid field address.
>> +     So to actually get all parent's pid and own pid/tid it requires to use
>> +     the syscalls.  */
>> +  pid_t ppid = getppid ();
>> +  pid_t pid = getpid ();
>> +  pid_t tid = syscall (__NR_gettid);
>>
>> +  while (write (pipefd[1], &ppid, sizeof ppid) < 0)
>> +    continue;
>>    while (write (pipefd[1], &pid, sizeof pid) < 0)
>>      continue;
>>    while (write (pipefd[1], &tid, sizeof tid) < 0)
> 
> These while loops look incorrect.  Perhaps just fail the test if the result is not equal to sizeof of the value being written?

I changed it.

> 
>> +  /* Some sanity checks for clone syscall: returned ppid should be currernt
> 
> “current”
> 

Ack.

> Thanks,
> Florian

In attachment I am sending a revised patch.

Remove cached PID/TID in clone

Commit Message

Comments

Patch