Date: Fri, 24 Sep 2010 22:53:01 -0400 From: Jung-uk Kim <jkim@FreeBSD.org> To: David Xu <davidxu@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: <201009242253.05383.jkim@FreeBSD.org> In-Reply-To: <4C9DBFD9.5000004@freebsd.org> References: <201009232220.o8NMK3fX011639@freefall.freebsd.org> <201009241943.10401.jkim@FreeBSD.org> <4C9DBFD9.5000004@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday 25 September 2010 05:24 am, David Xu wrote:
> 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.
Don't get me wrong, I have no intention to commit the patch. In fact,
I don't disagree with you. It was a proof-of-concept, friday night
experiment and that's about it. Probably you missed smiley in the
first patch. Sorry if I had wasted too much of your valuable
time. :-/
Jung-uk Kim
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201009242253.05383.jkim>