From patchwork Fri May 9 22:23:24 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Roland McGrath X-Patchwork-Id: 867 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (peon2454.g.dreamhost.com [208.113.200.127]) by wilcox.dreamhost.com (Postfix) with ESMTP id 356553600C0 for ; Fri, 9 May 2014 15:23:33 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14307373) id D7E7816FF719; Fri, 9 May 2014 15:23:32 -0700 (PDT) X-Original-To: glibc@patchwork.siddhesh.in Delivered-To: x14307373@homiemail-mx21.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx21.g.dreamhost.com (Postfix) with ESMTPS id A89D316FDA27 for ; Fri, 9 May 2014 15:23:32 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:mime-version:content-type :content-transfer-encoding:from:to:subject:message-id:date; q= dns; s=default; b=AnV8ZxgEQG/bgItiQwZ5kuxzp5ub2utxqO3PXUGVlxFwJB VsMl18Mh12BfUg2I6/l+KGv4vx7CgVnLOw/4YKAuhWIu1fgfPR4OaYPM2XTPSyJJ /uPP4rbcmMVbJXVZlfDlDfjhRGjBaaFBPBMjdz3MfvG+C/AFq9mB4FkDgQork= 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:mime-version:content-type :content-transfer-encoding:from:to:subject:message-id:date; s= default; bh=FEdlPzdM3HZ9wiBrS85I4V1qiwM=; b=egP/cQxFZIAAY170eA9D y6fPwML3qgpu0hG+pgNt0JmSl4EBxcKrRo9qjsb4kRSxAZ8MYgPqO6QpvLV9m9Wn RrzYwYFwkRZfvttw9Lvf0CSFhKxZ4I1UIrVuGvfN2M1sEJws1lz0rfqNgklrJd+4 TiPBLapeG8RGAbcxE6qEa9c= Received: (qmail 23750 invoked by alias); 9 May 2014 22:23:30 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 23647 invoked by uid 89); 9 May 2014 22:23:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00 autolearn=ham version=3.3.2 X-HELO: topped-with-meat.com MIME-Version: 1.0 From: Roland McGrath To: "GNU C. Library" Subject: [PATCH roland/getpid] Simplify getpid handling of the race case. Message-Id: <20140509222324.795042C3A01@topped-with-meat.com> Date: Fri, 9 May 2014 15:23:24 -0700 (PDT) X-CMAE-Score: 0 X-CMAE-Analysis: v=2.1 cv=J405smXS c=1 sm=1 tr=0 a=WkljmVdYkabdwxfqvArNOQ==:117 a=14OXPxybAAAA:8 a=hYMHPdPkQTkA:10 a=Z6MIti7PxpgA:10 a=kj9zAlcOel0A:10 a=hOe2yjtxAAAA:8 a=GAHKXb8dNqonzed0k08A:9 a=CjuIK1q_8ugA:10 X-DH-Original-To: glibc@patchwork.siddhesh.in In looking at consolidating the NPTL vs not versions of vfork, I came across the single, bizarre difference between the libpthread and libc versions of vfork. This is implemented by a few lines of assembly for each machine, and nowhere is there a comment that explains the rationale. The code in question is fiddling with the pid field of 'struct pthread'. It negates the value before doing vfork and then restores it afterwards in the parent. The rationale for this is that there could be a race where a signal handler runs in the child immediately after the vfork call, and in that case the getpid optimization of just returning THREAD_SELF->pid would return the parent's PID rather than the child's. That's all fair enough. Nothing ever sets ->pid in the child; it wouldn't be safe to do so, since the child and parent share memory, so if ->pid were ever >0 in the child, there would be a race after the child exits/execs and the parent resumes where a signal handler could run and see the child-set value of ->pid. (This just means that getpid's short-circuit logic will never win in a vfork child, so it makes actual getpid system calls.) The difference is that in the libc version, it checks for ->pid being zero and if it's zero instead sets it to 0x80000000 (i.e. an arbitrary negative number). The libpthread version doesn't do that, so if it's set to zero then it stays that way. AFAICT the reason for this is so that getpid, in its handling of the race case (i.e. THREAD_SELF->pid <= 0) can have another special case to avoid making the getpid system call: If ->pid was exactly zero but ->tid is nonzero, it just returns ->tid as the PID. But I don't actually see how ->pid could ever be set to zero so as to trigger this case. Then there is further logic whereby if ->pid and ->tid were both set to zero, then getpid will store the PID retrieved by the system call in ->tid (but not in ->pid). Again I don't see how that case can arise: ->tid can never be zero in a live thread after __pthread_initialize_minimal runs. Perhaps there can be a window where user constructors run before that? It all seems rather muddled, and I feel like I must be missing some of what's going on. If anything, the libc/libpthread difference seems backwards: in libc without libpthread, using ->tid should be OK because there's only one thread and its TID==PID; in libpthread, using ->tid can only be right if you're sure you're in the child and ->tid has been set to the child's new TID (which matches its PID). Maybe someone else can help explain it all. But all this complexity seems like a lot of hair to micro-optimize a case that can only arise during a race condition. If we simplify getpid so that it always just falls back to the system call when any fork/vfork is in progress, then we don't need the complexity and we don't need the libc and libpthread versions of vfork to differ. This change does that, leaving the way open to consolidate the vfork code during my NPTL consolidation. I'm not very clear on the utility of ever setting ->pid to a negative value. The fork and vfork code never look at the negated value again, they just save the original value locally and restore it later. So why not just zero ->pid at the beginning of fork and vfork instead? Does this seem sensible? Can anyone give a full explanation of the picture here that gives the rationale for the confusing things in the status quo? Thanks, Roland * nptl/sysdeps/unix/sysv/linux/getpid.c (really_getpid): Function removed. (__getpid): Rewritten. Under [!NOT_IN_libc], use THREAD_SELF->pid if it's > 0. Otherwise always just make the system call. --- a/nptl/sysdeps/unix/sysv/linux/getpid.c +++ b/nptl/sysdeps/unix/sysv/linux/getpid.c @@ -21,42 +21,17 @@ #include -#ifndef NOT_IN_libc -static inline __attribute__((always_inline)) pid_t really_getpid (pid_t oldval); - -static inline __attribute__((always_inline)) pid_t -really_getpid (pid_t oldval) -{ - if (__glibc_likely (oldval == 0)) - { - pid_t selftid = THREAD_GETMEM (THREAD_SELF, tid); - if (__glibc_likely (selftid != 0)) - 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 - pid_t __getpid (void) { -#ifdef NOT_IN_libc - INTERNAL_SYSCALL_DECL (err); - pid_t result = INTERNAL_SYSCALL (getpid, err, 0); -#else +#ifndef NOT_IN_libc pid_t result = THREAD_GETMEM (THREAD_SELF, pid); - if (__glibc_unlikely (result <= 0)) - result = really_getpid (result); + if (__glibc_likely (result > 0)) + return result; #endif - return result; + + INTERNAL_SYSCALL_DECL (err); + return INTERNAL_SYSCALL (getpid, err, 0); } libc_hidden_def (__getpid)