Date: Thu, 17 Jul 2014 03:56:38 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@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: <20140717005638.GF93733@kib.kiev.ua> In-Reply-To: <20140713213623.GA13241@dft-labs.eu> References: <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> <20140713213623.GA13241@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--z/MpV66pKPFGaNDq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Jul 13, 2014 at 11:36:24PM +0200, Mateusz Guzik wrote: > 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 fro= m the > > > > protected region, while sx-locked variant requires only one atomic = for > > > > entry and leave. > > > >=20 > > > > 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 dropp= ing > > > > 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 acquire= d, > > > > instead ? > > >=20 > > > Lock is taken later to avoid waiting for finished exec/exit of proces= ses > > > we cannot return, so that e.g. procstat -fa does not trip over that > > > much. > > >=20 > > > Right now only PROC_LOCK guarantees stability of p->p_ucred across pg= et > > > 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. > >=20 >=20 > Ignoring allproc_lock: >=20 > sx lock case: > filedesc out: slock + proc lock + proc unlock + sunlock > exit/exec: xlock + xunlock >=20 > counter case: > filedesc out: proc lock + proc unlock + proc lock + proc unlock > exit/exec: just wait for imagelock to be 0 This should be proc_lock/mwait/proc_lock, and proc_wait_imagelocked() does this. >=20 > 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 >=20 > I find the counter case better. >=20 > sx: > http://people.freebsd.org/~mjg/patches/sx-imagelock.patch >=20 > counter:=20 > http://people.freebsd.org/~mjg/patches/counter-imagelock.patch >=20 > There is an additional problem with slocked case: witness report a lor > with devfs vnodes. >=20 > 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 0xfffffe01232= 4f120 > 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 0xfffffe01= 2324f840 > sysctl_kern_proc_filedesc() at sysctl_kern_proc_filedesc+0x84/frame 0xfff= ffe012324f900 > sysctl_root_handler_locked() at sysctl_root_handler_locked+0x68/frame 0xf= ffffe012324f940 > sysctl_root() at sysctl_root+0x18e/frame 0xfffffe012324f990 > userland_sysctl() at userland_sysctl+0x192/frame 0xfffffe012324fa30 >=20 > witness detected the following lock orders: > devfs -> proctree Where is the dependency catched comes from ? I suspect it might be tty. I consider this case as an advantage of using sx over the hand-rolled lock, since the issue is/must be present with the counter as well, or the LOR is false positive, possibly due to sx taken in shared mode. But debugging features of sx give the warning, while counter is silent. That said, if the issue above is analyzed, I do not have any preference and will not object strongly against you decision. > proctree -> allproc > allproc -> imagelock > imagelock -> devfs --z/MpV66pKPFGaNDq Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTxx9GAAoJEJDCuSvBvK1BSsgP/283TvnZJTq7tDIIrtAcWqZX B50YNLs1ok6kNkNF5CeqcKZ8eepgAImnnrXyJWDN3fy70z2uBgHnLmYlWuIEFl2Y 6mx2PQEG/pERmxWsTx6snnWhCwmhX6YXpjZ8qVBWU28mCUuSlKdN0hEjBFgxY+TA +EdUbfGrI6ytsvzfbfYxc3rcWZATLwGAISRb9IsxDJS/8FW5NJ963WDjPRHdVhrY wFJJETCj5K22UlOwxXJAnDVDyGbHFSUvwyjhLreTzT/SNsu/48m3oRWBqwQgHvET 05SkXsyPTfmX1tb4MZ7a99txj4Lz3ouANHEAsxrszctJayHn7BB3iP3iOKRStR35 PgZsIGDDoQ6LZLLWtAMECZnI2hWB1J3/5MMA+HtWDVxAkIMYUL4YmbweA+Xz9SaW xYIuxIRLz209A3WPtNFbQFrgT56/AMW/R6XjThvAVSUpiG4KuDlGghIfTY/tBhHq oxrv9Hg2jikL3fpYr231r4wms26XUURHpUHPSoBJgA9aKDbvfV4Qh95tpgEAYLZv e0PsDU5Z5snHSTbpje4uMQVR/UisJ6jGbOHKjeaJC0Wxqxpv6cdIyUQiBhT/fk6g r04D1F5+jHFJtopoNiner1nc7BTnL7tWG3F+Lv0PGIUEVzIGaf6ew2H6G84FpUk9 ja6Ty0dsc302dsK3T3// =Y4qb -----END PGP SIGNATURE----- --z/MpV66pKPFGaNDq--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140717005638.GF93733>