Date: Sun, 13 Jul 2014 23:36:24 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Konstantin Belousov <kostikbel@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@FreeBSD.org> Subject: Re: svn commit: r267760 - head/sys/kern Message-ID: <20140713213623.GA13241@dft-labs.eu> In-Reply-To: <20140713132652.GZ93733@kib.kiev.ua> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140713213623.GA13241>