svn commit: r267760 - head/sys/kern

[Konstantin Belousov]

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 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
This should  be proc_lock/mwait/proc_lock, and proc_wait_imagelocked()
does this.

> 
> 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
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20140717/39509319/attachment-0001.sig>