From owner-svn-src-all@FreeBSD.ORG Sun Jul 13 21:36:30 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 C3C4BE11; Sun, 13 Jul 2014 21:36:30 +0000 (UTC) Received: from mail-wg0-x234.google.com (mail-wg0-x234.google.com [IPv6:2a00:1450:400c:c00::234]) (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 DE8642B20; Sun, 13 Jul 2014 21:36:29 +0000 (UTC) Received: by mail-wg0-f52.google.com with SMTP id a1so2496152wgh.11 for ; Sun, 13 Jul 2014 14:36:27 -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=R0NlDOMVxRjQbc6FypZZUPqTu7j5Ar3CsJ3mHD907FI=; b=xRzu93oqco9suy+oMiIQ32xWSMofvIdCzSqcNhvwIKh6PdqQbHSIee87Sd7aUaogh3 STkiACOM70pOAJk3HDTF+KSc/H7ca2Rkpwwabm2Sc884wohi32Nb+8D2GmaPfahr5m8Q yqn3pQAYPKKbXR6GiMbV71RMO/cShTmsgmuFWMjdDPfCD/WItIS6Vrz8vg/FCdpeL+43 R8ZMcUvT45BauEydqBdpqxmTJGXhAswfc0jF+ZIwCitslOzJlhorxzCpAmo8WPTP7O9O GQ4K9hTf1fGpebC/g81drqERomVzHsfC6sQ9AOrb0qoHmJ5KrBYAG2mS9wKyJ98QTXfh lrMg== X-Received: by 10.180.24.66 with SMTP id s2mr19373544wif.33.1405287387254; Sun, 13 Jul 2014 14:36:27 -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 lq17sm23153373wic.19.2014.07.13.14.36.25 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 13 Jul 2014 14:36:26 -0700 (PDT) Date: Sun, 13 Jul 2014 23:36:24 +0200 From: Mateusz Guzik To: Konstantin Belousov Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20140713213623.GA13241@dft-labs.eu> References: <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> <20140711111925.GB18214@dft-labs.eu> <20140713132652.GZ93733@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20140713132652.GZ93733@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: Sun, 13 Jul 2014 21:36:30 -0000 On Sun, Jul 13, 2014 at 04:26:52PM +0300, Konstantin Belousov wrote: > On Fri, Jul 11, 2014 at 01:19:25PM +0200, Mateusz Guzik wrote: > > On Fri, Jul 11, 2014 at 12:55:51PM +0300, Konstantin Belousov wrote: > > > 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. > No, you could get both locks, imagelock first, proc_lock next. > Ignoring allproc_lock: sx lock case: filedesc out: slock + proc lock + proc unlock + sunlock exit/exec: xlock + xunlock counter case: filedesc out: proc lock + proc unlock + proc lock + proc unlock exit/exec: just wait for imagelock to be 0 counter can result in temporary errors due to catching the process in exec, on the other hand slock before proc lock forces the caller to wait even for processes it cannot read I find the counter case better. sx: http://people.freebsd.org/~mjg/patches/sx-imagelock.patch counter: http://people.freebsd.org/~mjg/patches/counter-imagelock.patch There is an additional problem with slocked case: witness report a lor with devfs vnodes. lock order reversal: 1st 0xfffff80006fe6ab0 process imagelock (process imagelock) @ /usr/src/sys/kern/kern_proc.c:287 2nd 0xfffff80018c88240 devfs (devfs) @ /usr/src/sys/kern/vfs_cache.c:1241 KDB: stack backtrace: db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe012324f120 kdb_backtrace() at kdb_backtrace+0x39/frame 0xfffffe012324f1d0 witness_checkorder() at witness_checkorder+0xdc2/frame 0xfffffe012324f260 __lockmgr_args() at __lockmgr_args+0x588/frame 0xfffffe012324f3a0 vop_stdlock() at vop_stdlock+0x3c/frame 0xfffffe012324f3c0 VOP_LOCK1_APV() at VOP_LOCK1_APV+0xfc/frame 0xfffffe012324f3f0 _vn_lock() at _vn_lock+0xaa/frame 0xfffffe012324f460 vn_vptocnp_locked() at vn_vptocnp_locked+0xe8/frame 0xfffffe012324f4d0 vn_fullpath1() at vn_fullpath1+0xb0/frame 0xfffffe012324f530 vn_fullpath() at vn_fullpath+0xc1/frame 0xfffffe012324f580 export_fd_to_sb() at export_fd_to_sb+0x489/frame 0xfffffe012324f7b0 kern_proc_filedesc_out() at kern_proc_filedesc_out+0x1c6/frame 0xfffffe012324f840 sysctl_kern_proc_filedesc() at sysctl_kern_proc_filedesc+0x84/frame 0xfffffe012324f900 sysctl_root_handler_locked() at sysctl_root_handler_locked+0x68/frame 0xfffffe012324f940 sysctl_root() at sysctl_root+0x18e/frame 0xfffffe012324f990 userland_sysctl() at userland_sysctl+0x192/frame 0xfffffe012324fa30 witness detected the following lock orders: devfs -> proctree proctree -> allproc allproc -> imagelock imagelock -> devfs -- Mateusz Guzik