Date: Sat, 25 Sep 2010 09:24:41 +0000 From: David Xu <davidxu@freebsd.org> To: Jung-uk Kim <jkim@freebsd.org> Cc: Daniel Eischen <deischen@freebsd.org>, freebsd-threads@freebsd.org Subject: Re: threads/150889: PTHREAD_MUTEX_INITIALIZER + pthread_mutex_destroy () == EINVAL Message-ID: <4C9DBFD9.5000004@freebsd.org> In-Reply-To: <201009241943.10401.jkim@FreeBSD.org> References: <201009232220.o8NMK3fX011639@freefall.freebsd.org> <201009241137.56764.jkim@FreeBSD.org> <201009241724.10223.jhb@freebsd.org> <201009241943.10401.jkim@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Jung-uk Kim wrote:
> On Friday 24 September 2010 05:24 pm, John Baldwin wrote:
>> On Friday, September 24, 2010 11:37:53 am Jung-uk Kim wrote:
>>> On Friday 24 September 2010 09:26 am, John Baldwin wrote:
>>>> On Thursday, September 23, 2010 11:48:40 pm Jung-uk Kim wrote:
>>>>> On Thursday 23 September 2010 06:44 pm, Daniel Eischen wrote:
>>>>>> You shouldn't have to call pthread_mutex_init() on a mutex
>>>>>> initialized with PTHREAD_MUTEX_INITIALIZER. Our
>>>>>> implementation should auto initialize the mutex when it is
>>>>>> first used; if it doesn't, I think that is a bug.
>>>>> Ah, I see. I verified that libthr does it correctly.
>>>>> However, that's a hack and it is far from real static
>>>>> allocation although it should work pretty well in reality,
>>>>> IMHO. More over, it will have a side-effect, i.e., any
>>>>> destroyed mutex may be resurrected if it is used again.
>>>>> POSIX seems to say it should return EINVAL when it happens.
>>>>> :-(
>>>> I think the fix there is that we should put a different value
>>>> ((void *)1 for example) into "destroyed" mutex objects than 0
>>>> so that destroyed mutexes can be differentiated from statically
>>>> initialized mutexes. This would also allow us to properly
>>>> return EBUSY, etc.
>>> It would be nice if we had "uninitialized" as (void *)0 and
>>> "static initializer" as (void *)1. IMHO, it looks more natural,
>>> i.e., "uninitialized" or "destroyed" one gets zero, and
>>> "dynamically initialized" or "statically initialized" one gets
>>> non-zero. Can we break the ABI for 9, maybe? ;-)
>> I actually find the (void *)1 more natural for a destroyed state
>> FWIW. One thing I would advocate is that regardless of the values
>> chosen, use constants for both the INITIALIZER and DESTROYED
>> values. That would make the code more obvious. In general I think
>> your patch in your followup is correct, but having explicitly
>> DESTROYED constants that you check against instead of NULL would
>> improve the code.
>
> Here goes more complicated patch. Not really tested, though.
>
> Jung-uk Kim
>
The patch touches too many lines which is unnecessary to be modified,
it seems you are arbitrarily restructure the code according to you
hobby. Second, breaking ABI compatible is not accepted, I think jhb@ is
right here, destroyed mutex should have pointer 1 or others,
non-null PTHREAD_MUTEX_INITIALIZER causes data to be put in data
segment not in bss, result is large binary size, if it can be avoided,
it should be avoided. Third, inserting extra branch in critical path
should be caution, it is visible in performance losting in past I have
compared it with competitor. I think to minimize abnormal case,
I may use
if (((uintptr_t)mutex) <= 1) {
if (mutex == NULL) init it.
else if (mutex == destroyed)
return EINVAL;
}
This should have same overhead with current code.
Regards,
David Xu
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4C9DBFD9.5000004>