[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1409497642.4476.289.camel@eris.loria.fr>
Date: Sun, 31 Aug 2014 17:07:22 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: [PATCH 7/8] add the thrd_xxxxxx functions
Am Sonntag, den 31.08.2014, 10:05 -0400 schrieb Rich Felker:
> On Sun, Aug 31, 2014 at 03:19:21PM +0200, Jens Gustedt wrote:
> > Am Sonntag, den 31.08.2014, 08:57 -0400 schrieb Rich Felker:
> > > On Sun, Aug 31, 2014 at 09:57:34AM +0200, Jens Gustedt wrote:
> > > > > > if (attrp) attr = *attrp;
> > > > > >
> > > > > > __acquire_ptc();
> > > > > > @@ -234,7 +253,10 @@ static int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restr
> > > > > > new->canary = self->canary;
> > > > > >
> > > > > > a_inc(&libc.threads_minus_1);
> > > > > > - ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> > > > > > + if (c11)
> > > > > > + ret = __clone(start_c11, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> > > > > > + else
> > > > > > + ret = __clone(start, stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> > > > >
> > > > > Couldn't this be "c11 ? start_c11 : start" to avoid duplicating the
> > > > > rest of the call?
> > > >
> > > > I think that the ternary expression together with the other
> > > > parenthesized paramenter and the length of the line would make the
> > > > line barely readable.
> > > >
> > > > This are some of the pivots lines of the implementation, I'd rather
> > > > have them stick out.
> > > >
> > > > Also the assembler that is produced should be identical.
> > >
> > > Whether or not the output is identical, your code is much less
> > > readable and maintainable: an active effort is required by the reader
> > > to determine that the only difference between the two code paths is
> > > the function pointer being passed. This is why I prefer the use of ?:.
> > >
> > > The following looks perfectly readable to me:
> > >
> > > ret = __clone(c11 ? start_c11 : start, stack, flags,
> > > new, &new->tid, TP_ADJ(new), &new->tid);
> >
> > No to me (seriously!) because my builtin parser has difficulties to
> > capture the end of the ternary expression.
> >
> > Would it be acceptable to have parenthesis around?
>
> I wouldn't even mind if you'd prefer to rename start to start_pthread
> and then make start a local variable:
>
> start = c11 ? start_c11 : start_pthread;
> ret = __clone(start, stack, flags, new,
> &new->tid, TP_ADJ(new), &new->tid);
>
> Would that be nicer?
I opted for the parenthesis version, now.
> > > > > Also, since it doesn't depend on anything in pthread_create.c,
> > > >
> > > > It does, __THRD_C11 :)
> > >
> > > How about naming it to __ATTRP_C11_THREAD and putting it in
> > > pthread_impl.h then?
> >
> > ok, I'll do that
> >
> > But hopefully we didn't overlook any possible use pattern that would
> > drag in different versions of the tsd symbols. I am not too
> > comfortable with that.
>
> What do you mean here? I don't follow.
When trying to separate the two implementations, for a while I
struggled with the fact that the game with the dummy functions
enforces that pthread_exit and pthread_create must be in the same
TU. I found that difficult to deal with, not knowing the code and the
interaction between the different TU too well.
(There might be an independent possibility of improvement in
readability and maintainability in separating pthread_create and
pthread_exit more cleanly.)
> > > Yes I'm aware, but I don't want gratuitous incentives for patch 8/8
> > > just because patch 7/8 is done intentionally suboptimally. :-) If the
> > > approaches are being compared, we should be comparing the preferred
> > > efficient versions of both. And I'd like to wait to seriously think
> > > about 8/8 until everything else is fully ready to commit, or
> > > preferably already committed.
> >
> > I know.
> >
> > But I just discovered another such incentive :) You were right, that
> > the error handling for thrd_create was not correct for C11, but it
> > wasn't my fault :) POSIX (and thus __pthread_create) basically maps
> > all errors to EAGAIN, where C11 requires us to distinguish ENOMEM from
> > other failures.
> >
> > Thus I had to integrate this difference into __pthread_create, which
> > was not difficult, but which intrudes even a little bit more into the
> > existing code.
>
> I think POSIX's EAGAIN is fully equivalent to C11's thrd_nomem: both
> should reflect any condition where the thread could not be created due
> to a resource exhaustion type failure. While you could argue that
> thrd_nomem should only indicate failure of the mmap, not the clone, I
> think this would be a useless distinction to callers (both of them are
> fundamentally allocation operations) and then you'd be forced to use
> thrd_error for clone failures, whereas I would think thrd_error should
> be reserved for either erroneous usage (but that's UB anyway) or more
> permanent failures (like lack of thread support on the OS).
Having read up a bit, now, I don't think so, for C threads this
mapping is not correct. clone has several different error returns
that the actual code correctly maps to EAGAIN, but among them it also
has ENOMEM.
So we have possible ENOMEM coming from clone and from mmap, and a
bunch of other obscure errors that should go to thrd_error.
> > > I'm aware that you can't cast the int* to void**, but you can still
> > > implement the function as a trivial wrapper that doesn't introduce any
> > > duplication of internal logic for cleaning up an exited thread:
> > >
> > > int thrd_join(thrd_t t, int *res)
> > > {
> > > void *pthread_res;
> > > __pthread_join(t, &pthread_res);
> > > if (res) *res = (int)(intptr_t)pthread_res;
> > > return thrd_success;
> > > }
> >
> > dunno, doesn't look much simpler to me and drags in one more TU into C
> > thread applications
>
> What's simpler is that this version does not:
>
> - Need pthread_impl.h
> - Have knowledge of the mechanism for waiting for thread exit.
> - Have knowledge of how to obtain the exit code out of thread struct.
> - Have knowledge of thread deallocation mechanism.
Ok, I'll move that separated implementation to patch 8, and use your
version for patch 7.
Jens
--
:: INRIA Nancy Grand Est ::: AlGorille ::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536 ::
:: :::::::::::::::::::::: gsm France : +33 651400183 ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::
Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
