[PATCH] Remove cached PID/TID in clone
Adhemerval Zanella
adhemerval.zanella@linaro.org
Tue Nov 8 19:58:00 GMT 2016
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/nptl_db/structs.def b/nptl_db/structs.def
index a9b621b..1cb6a46 100644
--- a/nptl_db/structs.def
+++ b/nptl_db/structs.def
@@ -48,7 +48,6 @@ DB_STRUCT (pthread)
DB_STRUCT_FIELD (pthread, list)
DB_STRUCT_FIELD (pthread, report_events)
DB_STRUCT_FIELD (pthread, tid)
-DB_STRUCT_FIELD (pthread, pid)
DB_STRUCT_FIELD (pthread, start_routine)
DB_STRUCT_FIELD (pthread, cancelhandling)
DB_STRUCT_FIELD (pthread, schedpolicy)
>
>> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-cached-PID-TID-in-clone.patch
Type: text/x-patch
Size: 74329 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20161108/3ef4caa1/attachment.bin>
More information about the Libc-alpha
mailing list