From owner-svn-src-head@freebsd.org Sat Feb 17 16:07:08 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6AE4BF1BF78; Sat, 17 Feb 2018 16:07:08 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-qk0-x235.google.com (mail-qk0-x235.google.com [IPv6:2607:f8b0:400d:c09::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 045E087075; Sat, 17 Feb 2018 16:07:08 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-qk0-x235.google.com with SMTP id s198so7482909qke.5; Sat, 17 Feb 2018 08:07:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8myZr/uqljKLWmMNym+YWvpvLj1ff3EOYXzQLOs3wyo=; b=RMirQ4r7W+Q5Zma6jeDnbH0c2MBUm/S5oQKYsE0cj6H6Swcf1aWpqzqLiWiINKJ2ir UFsdrg6OkijUaVejcaahqZ5jpdrQpInjC2y3qfDfRzPSv7NIXodpQXcpyQkIDvwTz/WZ xX6XhhqGJxwCFeqE9oYde0iVCjFrGYZAfVX698AM7AanhfH68UUAyLN/ZHUj7npM8ZCp 9FCbA5x9DL9EdKR/Vyysda1SgCdqlAW27yd5UlYOESgomjj6hPn942JVMw+KPmiKlj3D DbmAD9JMN5/IPdKLsm52sqLHZR9uqJPtDbx5s0arhGDuvdDkKWzak1VdRZ29ZNaOHDq2 7/FA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8myZr/uqljKLWmMNym+YWvpvLj1ff3EOYXzQLOs3wyo=; b=MgEOCPaaoENYr6JvTxIu2TW6co4b8CsgY6yk3FI0IVZR8hYKV8AnT/c0a/5X06URTH ocV2QX22hCuMqFB6tdhi0JM0VMkdKHDsJTan18+829Dfqh46Lf+wU7C7XgeBrRLB/FdB VNimOpwc5am78r+DAmZkbC8kTIMz2CoxoL9ozNUH2jaSYmPcwXo0r6qisxuhaJxdd6En DpD5/0+mai7IJTHGhBesuovmhl5jeE0sHWXz0JEVqUDkUYQmDdmHPSCrOVsqpYbY5WE1 aHo4o7RUZvD+Vo7nXye5uhLih/KL0kez61AYrjJFHfF3YHIOml0QXSX2h70jFTWQYyzL Vd0g== X-Gm-Message-State: APf1xPCOf3SXNenMTdTEo9c/o0NriljRtkXollahn8C3RfkH1W77/man SByOCqzOmxIdS2SELX/gVzN7Wil3G0WhFEHlPwrNPA== X-Google-Smtp-Source: AH8x225G34vMYbt0DgWgvukbr1M3Sa54Yi1C8495DmUFWm/BdYigTy/I+I2P+fT+AziA0LvFbS4SSm0pNjuvFwsMCJk= X-Received: by 10.55.22.144 with SMTP id 16mr14774785qkw.11.1518883627554; Sat, 17 Feb 2018 08:07:07 -0800 (PST) MIME-Version: 1.0 Received: by 10.237.58.99 with HTTP; Sat, 17 Feb 2018 08:07:07 -0800 (PST) In-Reply-To: <20180217112738.GO94212@kib.kiev.ua> References: <201802170848.w1H8mkfb081764@repo.freebsd.org> <20180217112738.GO94212@kib.kiev.ua> From: Mateusz Guzik Date: Sat, 17 Feb 2018 17:07:07 +0100 Message-ID: Subject: Re: svn commit: r329448 - head/sys/kern To: Konstantin Belousov Cc: Mateusz Guzik , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 17 Feb 2018 16:07:08 -0000 On Sat, Feb 17, 2018 at 01:27:38PM +0200, Konstantin Belousov wrote: > On Sat, Feb 17, 2018 at 08:48:46AM +0000, Mateusz Guzik wrote: > > Author: mjg > > Date: Sat Feb 17 08:48:45 2018 > > New Revision: 329448 > > URL: https://svnweb.freebsd.org/changeset/base/329448 > > > > Log: > > exit: get rid of PROC_SLOCK when checking a process to report > Was this tested ? > I was trussing multithreaded microbenchmarks, no issues. > In particular, are you aware of r309539 ? > So it looks like I misread the code - I have grepped thread_suspend_switch operating with the proc locked and misread thread_suspend_one's assert as PROC_LOCK_ASSERT. That said, I think this is harmless. Regardless of the lock the inspecting thread can race and check "too soon". Even for a case where it decides to report, I don't see anything which would depend on the suspending thread to finish. However, locking can be employed in a way which is avoided in the common case: diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c index b063bda5b7ff..4ae24bcd7059 100644 --- a/sys/kern/kern_exit.c +++ b/sys/kern/kern_exit.c @@ -1174,6 +1174,7 @@ kern_wait6(struct thread *td, idtype_t idtype, id_t id, int *status, struct proc *p, *q; pid_t pid; int error, nfound, ret; + bool report; AUDIT_ARG_VALUE((int)idtype); /* XXX - This is likely wrong! */ AUDIT_ARG_PID((pid_t)id); /* XXX - This may be wrong! */ @@ -1226,27 +1227,36 @@ kern_wait6(struct thread *td, idtype_t idtype, id_t id, int *status, PROC_LOCK_ASSERT(p, MA_OWNED); if ((options & WTRAPPED) != 0 && - (p->p_flag & P_TRACED) != 0 && - (p->p_flag & (P_STOPPED_TRACE | P_STOPPED_SIG)) != 0 && - p->p_suspcount == p->p_numthreads && - (p->p_flag & P_WAITED) == 0) { + (p->p_flag & P_TRACED) != 0) { + PROC_SLOCK(p); + report = + ((p->p_flag & (P_STOPPED_TRACE | P_STOPPED_SIG)) && + p->p_suspcount == p->p_numthreads && + (p->p_flag & P_WAITED) == 0); + PROC_SUNLOCK(p); + if (report) { CTR4(KTR_PTRACE, "wait: returning trapped pid %d status %#x " "(xstat %d) xthread %d", p->p_pid, W_STOPCODE(p->p_xsig), p->p_xsig, p->p_xthread != NULL ? p->p_xthread->td_tid : -1); - report_alive_proc(td, p, siginfo, status, options, - CLD_TRAPPED); - return (0); + report_alive_proc(td, p, siginfo, status, + options, CLD_TRAPPED); + return (0); + } } if ((options & WUNTRACED) != 0 && - (p->p_flag & P_STOPPED_SIG) != 0 && - p->p_suspcount == p->p_numthreads && - (p->p_flag & P_WAITED) == 0) { - report_alive_proc(td, p, siginfo, status, options, + report_alive_proc(td, p, siginfo, status, + options, CLD_TRAPPED); + return (0); + } } if ((options & WUNTRACED) != 0 && - (p->p_flag & P_STOPPED_SIG) != 0 && - p->p_suspcount == p->p_numthreads && - (p->p_flag & P_WAITED) == 0) { - report_alive_proc(td, p, siginfo, status, options, - CLD_STOPPED); - return (0); + (p->p_flag & P_STOPPED_SIG) != 0) { + PROC_SLOCK(p); + report = (p->p_suspcount == p->p_numthreads && + ((p->p_flag & P_WAITED) == 0)); + PROC_SUNLOCK(p); + if (report) { + report_alive_proc(td, p, siginfo, status, + options, CLD_STOPPED); + return (0); + } } if ((options & WCONTINUED) != 0 && (p->p_flag & P_CONTINUED) != 0) { On Sat, Feb 17, 2018 at 12:27 PM, Konstantin Belousov wrote: > On Sat, Feb 17, 2018 at 08:48:46AM +0000, Mateusz Guzik wrote: > > Author: mjg > > Date: Sat Feb 17 08:48:45 2018 > > New Revision: 329448 > > URL: https://svnweb.freebsd.org/changeset/base/329448 > > > > Log: > > exit: get rid of PROC_SLOCK when checking a process to report > Was this tested ? > > In particular, are you aware of r309539 ? > > > > > All accessed fields are protected with already held process lock. > > > > Modified: > > head/sys/kern/kern_exit.c > > > > Modified: head/sys/kern/kern_exit.c > > ============================================================ > ================== > > --- head/sys/kern/kern_exit.c Sat Feb 17 08:12:35 2018 (r329447) > > +++ head/sys/kern/kern_exit.c Sat Feb 17 08:48:45 2018 (r329448) > > @@ -1228,15 +1228,11 @@ loop_locked: > > nfound++; > > PROC_LOCK_ASSERT(p, MA_OWNED); > > > > - if ((options & (WTRAPPED | WUNTRACED)) != 0) > > - PROC_SLOCK(p); > > - > > if ((options & WTRAPPED) != 0 && > > (p->p_flag & P_TRACED) != 0 && > > (p->p_flag & (P_STOPPED_TRACE | P_STOPPED_SIG)) != 0 && > > p->p_suspcount == p->p_numthreads && > > (p->p_flag & P_WAITED) == 0) { > > - PROC_SUNLOCK(p); > > CTR4(KTR_PTRACE, > > "wait: returning trapped pid %d status %#x " > > "(xstat %d) xthread %d", > > @@ -1251,13 +1247,10 @@ loop_locked: > > (p->p_flag & P_STOPPED_SIG) != 0 && > > p->p_suspcount == p->p_numthreads && > > (p->p_flag & P_WAITED) == 0) { > > - PROC_SUNLOCK(p); > > report_alive_proc(td, p, siginfo, status, options, > > CLD_STOPPED); > > return (0); > > } > > - if ((options & (WTRAPPED | WUNTRACED)) != 0) > > - PROC_SUNLOCK(p); > > if ((options & WCONTINUED) != 0 && > > (p->p_flag & P_CONTINUED) != 0) { > > report_alive_proc(td, p, siginfo, status, options, > -- Mateusz Guzik