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.

Message ID	d0b874df-d9f7-6121-6786-6ac1292a82ec@linaro.org
State	Accepted
Commit	c579f48edba88380635ab98cb612030e3ed8691e
Headers	show Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp1728262qge; Tue, 8 Nov 2016 11:59:19 -0800 (PST) X-Received: by 10.99.172.26 with SMTP id v26mr9203399pge.164.1478635159098; Tue, 08 Nov 2016 11:59:19 -0800 (PST) Return-Path: <libc-alpha-return-74529-patch=linaro.org@sourceware.org> Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id y133si10481723pfb.72.2016.11.08.11.59.18 for <patch@linaro.org> (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Nov 2016 11:59:19 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-74529-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of libc-alpha-return-74529-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-74529-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-type; q=dns; s=default; b=Nmc9 Di998bAEyzRtIRaD6oIXSMpLoUHO2ivB6WpK6fdMriiCAyI6UiENsnmMa8CFKSEN gZxzH1WRCgvBGrCzV5BWDOr1yvUo0FZm3jZzHYs4pT6PoJ/nJdPhLSe1BXKgYhoa 8GSzA08STcctsRo2rBoyMTLtg2bqzjPOncvUfBw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-type; s=default; bh=YqdA8spaBh 3YTPCTwHa+VKcCbFA=; b=evq2Cp+pbhTNOySSsXMYIM91YFn8IbMKZivDBlvgFI 5y/pLaiTryLla29W/kzxgqkXuoi4nRIneL6DlQcq+7Iw6D2DS+tV/19lMw6pyOL0 Pm2himRwACKGPQ/Fy53GMglNX5MinmYu56uaZ8XgNsxivqz1ijYR+F6vRnp65E17 w= Received: (qmail 65542 invoked by alias); 8 Nov 2016 19:58:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-patch=linaro.org@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 65524 invoked by uid 89); 8 Nov 2016 19:58:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=3.3 required=5.0 tests=BAYES_50, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=drepper, shad, sp, 96 X-HELO: mail-ua0-f182.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=mhBuc/VGKHkk3jG5K71noGE8YEDLrN2XBT6YAGtsLPk=; b=QUU3P4uhR6nffz+cMOVpZwkdyMe4wTfIXUdoeEtY0iUgxDbcNsLW30nlOpG5cce86B 5b2giTXCVQ3g4A5sVoGRF+olbwZYECKCseG9t4Z3muP1eQJh0WKQibNMD2tLZ2dFyIlI X7vvQWbyST3BP/cp4MCgvOoK2CQxHcfAabO6Fw4T+KudRDzDta3C6Eue+B8S1Cd1MA/R ZqQ7sxjH/HdQ9/c9+uFiHR2Ss+Ym1GBNgSu8X/mNGCqQnl2cktVkCIeNG5PonpFPKDbp j5iWj/huoGTnmOZEFsktrcqFuYAApBHYjcKoMwBa0xciN7Mh72uEYSh4yzHbkGpKy/oq hjwA== X-Gm-Message-State: ABUngvfUDJeJeAL3W8ueZ6tqZoUlbY62JmGBMILMtpNKbzfnvXnvGYhljxm4ee+nxIsfLwmM X-Received: by 10.176.85.75 with SMTP id u11mr800068uaa.85.1478635124033; Tue, 08 Nov 2016 11:58:44 -0800 (PST) Subject: Re: [PATCH] Remove cached PID/TID in clone To: Florian Weimer <fweimer@redhat.com>, libc-alpha@sourceware.org References: <1476387924-4509-1-git-send-email-adhemerval.zanella@linaro.org> <c432ff40-edf2-1ebe-f3a7-de76fbfdd252@redhat.com> From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Message-ID: <d0b874df-d9f7-6121-6786-6ac1292a82ec@linaro.org> Date: Tue, 8 Nov 2016 17:58:38 -0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <c432ff40-edf2-1ebe-f3a7-de76fbfdd252@redhat.com> Content-Type: multipart/mixed; boundary="------------951D579DA48B3303E2D4668C"

Remove cached PID/TID in clone

Commit Message

Comments

Patch