Date: Mon, 9 May 2016 05:51:07 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Jilles Tjoelker <jilles@stack.nl> Cc: threads@freebsd.org, arch@freebsd.org Subject: Re: Robust mutexes implementation Message-ID: <20160509025107.GN89104@kib.kiev.ua> In-Reply-To: <20160508125222.GA48862@stack.nl> References: <20160505131029.GE2422@kib.kiev.ua> <20160506233011.GA99994@stack.nl> <20160507165956.GC89104@kib.kiev.ua> <20160508125222.GA48862@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, May 08, 2016 at 02:52:22PM +0200, Jilles Tjoelker wrote: > OK. The patch still initializes umtx_shm_vnobj_persistent to 1 though. > There is also a leak if umtx_shm_vnobj_persistent is toggled from 1 to 0 > while an unmapped object with an off-page is active. > > I forgot two issues in my analysis. > > Firstly, the automatic re-initialization (from r297185, in fact) allows > the next thread to lock the mutex even if the previous thread terminated > while holding the lock. Normally, one would expect that non-robust > mutexes cause the lock attempt to hang (or fail if it's a trylock), > except if a thread ID is reused (in which case the lock attempt may > instead succeed recursively or fail). > > Secondly, the initialization attributes are lost. If the next thread > after last unmap sees THR_PSHARED_PTR, it has no idea whether it should > be robust or not, which type it should have and whether it should be > PP/PI. A mutex that was originally initialized as non-robust should not > be re-initialized as robust since the code will not handle the > [EOWNERDEAD] and [ENOTRECOVERABLE] errors (in the worst case, errors are > completely ignored and it is as if the mutex did not exist). > > Special-casing robust to store in the THR_PSHARED_PTR location seems > strange since the other attributes are still lost. Yes, these are all good points. > > All this is POSIX-compliant since POSIX specifies that the state of > synchronization objects becomes undefined on last unmap, and our > implementation fundamentally depends on that possibility. Unfortunately, Could you, please, point me to the exact place in the standard where this is allowed ? > Linux and Solaris do not need the possibility. The automatic > re-initialization and umtx_vnode_persistent are just hacks that make > certain applications almost always work (but not always, and in such > cases it will be hard to debug). > > Another issue with umtx_vnode_persistent is that it can hide high memory > usage. Filling up a page with pthread_mutex_t will create many pages > full of actual mutexes. This memory usage is only visible as long as it > is still mapped somewhere. There is already a resource limit for the number of pshared locks per uid, RLIMIT_UMTXP. When exceeded, user would get somewhat obscure failure mode, but excessive memory consumption is not allowed. And I think that vmstat -o would give enough info to diagnose, except that users must know about it and be quialified enough to interpret the output. > > Apart from that, umtx_vnode_persistent can (at least conceptually) work > fully reliably for shared memory objects and tmpfs files, which do not > have persistent storage. I changed defaults for the umtx_vnode_persistent to 0 in the published patch. > Hmm, libthr2 or non-standard synchronization primitive implementations > seem a good reason to not check for umtx shm page. > > However, the existing checks can be made stricter. The umtx_handle_rb() > from robust.3.patch will use m_rb_lnk with no validation at all except > that it is a valid pointer. However, if the UMUTEX_ROBUST flag is not > set, the mutex should not have been in this list at all and it is > probably safer to ignore m_rb_lnk. Ok, I changed the code to consider lack of UMUTEX_ROBUST as a stopper for the list walk. Also, I stop the walk if mutex is not owned by the current thread, except when the mutex was stored in inact slot. The same piece of changes hopefully fixes list walk for COMPAT32 on big-endian machines. > > There is a difference between chunked allocations and the current > m_rb_lnk in that the list would reside in local memory, not vulnerable > to other processes scribbling over it. This is probably not a major > issue since sharing a mutex already allows threads to block each other > indefinitely. I would easily deletegate the chunked array to some future reimplementation if not the ABI issue. Still, I do not like it. Current updates to the patch https://kib.kiev.ua/kib/pshared/robust.4.patch
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160509025107.GN89104>