svn commit: r302194 - head/lib/libthr/thread

[Daniel Eischen]

On Sat, 25 Jun 2016, Konstantin Belousov wrote:

> On Sat, Jun 25, 2016 at 07:14:40PM +0200, Jilles Tjoelker wrote:
>> On Sat, Jun 25, 2016 at 11:30:40AM +0000, Konstantin Belousov wrote:
>>> Author: kib
>>> Date: Sat Jun 25 11:30:40 2016
>>> New Revision: 302194
>>> URL: https://svnweb.freebsd.org/changeset/base/302194
>>
>>> Log:
>>>   For pthread_mutex_trylock() call on owned error-check or non-portable
>>>   adaptive mutex, return EDEADLK as required by POSIX.  The
>>>   pthread_mutex_lock() is already compliant.
>>
>>>   Tested by:	Guy Yur <guyyur at gmail.com>
>>>   Sponsored by:	The FreeBSD Foundation
>>>   MFC after:	2 weeks
>>>   Approved by:	re (gjb)
>>
>>> Modified:
>>>   head/lib/libthr/thread/thr_mutex.c
>>
>>> Modified: head/lib/libthr/thread/thr_mutex.c
>>> ==============================================================================
>>> --- head/lib/libthr/thread/thr_mutex.c	Sat Jun 25 10:08:04 2016	(r302193)
>>> +++ head/lib/libthr/thread/thr_mutex.c	Sat Jun 25 11:30:40 2016	(r302194)
>>> @@ -850,9 +850,12 @@ mutex_self_trylock(struct pthread_mutex
>>>
>>>  	switch (PMUTEX_TYPE(m->m_flags)) {
>>>  	case PTHREAD_MUTEX_ERRORCHECK:
>>> -	case PTHREAD_MUTEX_NORMAL:
>>>  	case PTHREAD_MUTEX_ADAPTIVE_NP:
>>> -		ret = EBUSY;
>>> +		ret = EDEADLK;
>>> +		break;
>>> +
>>> +	case PTHREAD_MUTEX_NORMAL:
>>> +		ret = EBUSY;
>>>  		break;
>>>
>>>  	case PTHREAD_MUTEX_RECURSIVE:
>>
>> I think POSIX (SUSv4tc1, XSH 3 pthread_mutex_lock) is clear that only
>> pthread_mutex_lock() can fail with [EDEADLK], not
>> pthread_mutex_trylock(). Instead, the error [EBUSY] listed for
>> pthread_mutex_trylock() applies whenever the mutex could not be acquired
>> because it was already locked. This includes the case where the mutex is
>> owned by the current thread. Note that POSIX intends to allow not
>> storing the owning thread's ID in non-recursive non-robust mutexes.
>>
>> Failing pthread_mutex_trylock() on owned mutexes only with [EBUSY] also
>> matches our code before this commit and NetBSD's code, and is apparently
>> expected by other code.
>>
>> Therefore, I think this commit should be reverted.
>>
>
> I already asked re for approval of the reversal and got it.  But I am still
> hesitating doing the revert vs. returning EDEADLK for error-checking
> mutexes.
>
> My initial mistake was reading the statement about PTHREAD_MUTEX_ERRORCHECK
> returning EDEADLK as the requirement for both functions.  It was induced
> by reading the following code in samba:
> https://github.com/samba-team/samba/blob/master/lib/tdb/common/mutex.c#L928
> I did extracted this into stand-alone test and checked that glibc does
> return EDEADLK in this case.  BTW, if somebody has Solaris machine available
> to test this, I would be grateful.  Code is available at
> https://www.kib.kiev.ua/kib/pshared/pthread_samba.c
>
> I.e., plain revert would disable the only known to me consumer of the
> robust mutexes. The patch which I mailed last time, returns EDEADLK for
> trylock on ERRORCHECKed mutexes only. And I am tending toward glibc
> compatibility there, over the literal POSIX compliance, but I want to
> see the confirmation from the Klimenko' test first.

That doesn't make glibc correct.  Unless the standards change,
we should return EBUSY in this case.  It is also unexpected if an
implementation's default mutex scheme is PTHREAD_MUTEX_ERRORCHECK
and it returns EDEADLK in this case.  It's not mentioned in the
standard, and no portable application will be expecting it.

-- 
DE