From owner-svn-src-all@FreeBSD.ORG Fri Jul 11 11:19:38 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E7B8B97C; Fri, 11 Jul 2014 11:19:37 +0000 (UTC) Received: from mail-wi0-x22c.google.com (mail-wi0-x22c.google.com [IPv6:2a00:1450:400c:c05::22c]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 0DFFB2D53; Fri, 11 Jul 2014 11:19:36 +0000 (UTC) Received: by mail-wi0-f172.google.com with SMTP id n3so61240wiv.17 for ; Fri, 11 Jul 2014 04:19:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=St4giY+FtVqKHwfNC2mt92T+EYtmUFgXHT3XmyfOyLw=; b=AngwHLSMPR4Maof46JsMZCfOS3EDEwI/IeEJsgwH9FyXsocohFFMUqGLHD0utkdBEg HaLWKZJ+4/yF4QjkRyIYM0GU5nYfvWNFrvuw0Fp4hT5+qJeto7oSusWivhb/bpJDO2WI iF4cLO0FhhNWDQE7kebemF/MBLmRDXLYkzTdromcIx3agRHeOfUrz+VITrgqK/EIbnWV MgTPNAq9Cgz3UAgGEkRMJ6h1zwl8sCA5SxIR3xLKE41RHr0iJe/AZVLE4LAqtfu8pKcw n5YrRrKa04I/9XPRGl9qZE4AENSlIlaj63KVQkUWFxxCXY1smSQUGjFFvf6nDNIBrTZr /BCg== X-Received: by 10.194.62.167 with SMTP id z7mr27979496wjr.112.1405077572265; Fri, 11 Jul 2014 04:19:32 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id n2sm4532893wjf.40.2014.07.11.04.19.30 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 11 Jul 2014 04:19:31 -0700 (PDT) Date: Fri, 11 Jul 2014 13:19:25 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20140711111925.GB18214@dft-labs.eu> References: <201406230128.s5N1SIYK097224@svn.freebsd.org> <20140623064044.GD93733@kib.kiev.ua> <20140623070652.GA27040@dft-labs.eu> <20140623072519.GE93733@kib.kiev.ua> <20140623080501.GB27040@dft-labs.eu> <20140623081823.GG93733@kib.kiev.ua> <20140623131653.GC27040@dft-labs.eu> <20140623163523.GK93733@kib.kiev.ua> <20140711024351.GA18214@dft-labs.eu> <20140711095551.GA93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140711095551.GA93733@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jul 2014 11:19:38 -0000 On Fri, Jul 11, 2014 at 12:55:51PM +0300, Konstantin Belousov wrote: > On Fri, Jul 11, 2014 at 04:43:51AM +0200, Mateusz Guzik wrote: > > In both cases the same mechanism blocks both exec and exit, this can be > > split if needed (p_lock would still cover exit, p_something would cover > > exec). > > > > Here is a version with sx lock: > > > > http://people.freebsd.org/~mjg/patches/exec-exit-hold-wait.patch > > > > I'm not really happy with this. Reading foreign fdt is very rare and > > this adds lock + unlock for every exec and exit. > > > > On the other hand mere counter version is rather simple: > > > > http://people.freebsd.org/~mjg/patches/exec-exit-hold-nolock.patch > > > > I don't have strong opinion here, but prefer the latter. > > I suggest the name 'imagelock' for the beast. > Sounds good. > The nolock version requires two atomics on both entry and leave from the > protected region, while sx-locked variant requires only one atomic for > entry and leave. > > I am not sure why you decided to acquire p->p_keeplock in after the > proc lock in pget(), which indeed causes the complications of dropping > the proc_lock and rechecking to avoid LOR. Did you tried to add a flag > to pfind*() functions to indicate that p_keeplock should be acquired, > instead ? Lock is taken later to avoid waiting for finished exec/exit of processes we cannot return, so that e.g. procstat -fa does not trip over that much. Right now only PROC_LOCK guarantees stability of p->p_ucred across pget operation. Without that the code would have to crget() and various functions modified to accept cred instead of proc, or 'imagelock' mechanism would have to be extended to also protect against cred changes. That said, the code could be indeed changed to sx requiring one atomic on entry and leave, but that would still leave us with such atomics in exit and exec and the last 2 are way more common than the first one, thus I prefer counter case which only adds lock + unlock on leave. -- Mateusz Guzik