From: Rich Felker <dalias@libc.org>
To: musl@lists.openwall.com
Subject: Re: [PATCH 7/8] add the thrd_xxxxxx functions
Date: Sun, 31 Aug 2014 08:57:45 -0400 [thread overview]
Message-ID: <20140831125745.GW12888@brightrain.aerifal.cx> (raw)
In-Reply-To: <1409471854.4476.272.camel@eris.loria.fr>
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);
> > 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?
> > it would be nice to put it in a separate thrd_create.c. It's not a big
> > deal but it shaves off a small function in POSIX programs that don't
> > use thrd_create.
>
> I'd really prefer to keep all the logic of thrd_create together in one
> file. All the other additions at this point are tightly bound to be in
> the same TU as pthread_create, for all this weak symbol stuff that is
> going on with tsd and friends.
>
> Also see that as an incentive to accept patch 8/8 to separate both
> implementations 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'd really just prefer that all of these can't-fail cases be a
> > straight tail call with no support for nonzero thrd_success values.
> > But as long as the code is correct and the inefficiency is trivially
> > optimized out, I'm not going to spend a lot of time arguing about it.
> > I do think it's telling, though, that the (albeit minimal) complexity
> > of trying to handle the case where thrd_success is nonzero seems to
> > have caused a couple bugs -- this is part of why I don't like having
> > multiple code paths where one path is untestable/untested. To me, a
> > code path that's never going to get tested is a much bigger offense
> > than an assumption that a constant has the value we decided it has.
Do you have any thoughts on this part?
> > > diff --git a/src/thread/thrd_join.c b/src/thread/thrd_join.c
> > > new file mode 100644
> > > index 0000000..a8c7aed
> > > --- /dev/null
> > > +++ b/src/thread/thrd_join.c
> > > @@ -0,0 +1,16 @@
> > > +#include "pthread_impl.h"
> > > +#include <sys/mman.h>
> > > +#include <threads.h>
> > > +
> > > +int __munmap(void *, size_t);
> > > +
> > > +/* C11 threads cannot be canceled, so there is no need for a
> > > + cancelation function pointer, here. */
> > > +int thrd_join(thrd_t t, int *res)
> > > +{
> > > + int tmp;
> > > + while ((tmp = t->tid)) __timedwait(&t->tid, tmp, 0, 0, 0, 0, 0);
> > > + if (res) *res = (int)(intptr_t)t->result;
> > > + if (t->map_base) __munmap(t->map_base, t->map_size);
> > > + return thrd_success;
> > > +}
> >
> > I'd rather avoid duplicating this function too.
>
> No this ain't a duplicate. The cast here is necessary and plain use of
> pthread_join would have us interpret an int* as void**, so the
> assignment could potentially overwrite the second half of the word res
> is pointing to.
>
> I'll have that visually stick out with a comment
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;
}
Rich
next prev parent reply other threads:[~2014-08-31 12:57 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-30 18:46 [PATCH 0/8] C thread patch series, v. 8.3 and 9.4 Jens Gustedt
2014-08-30 18:46 ` [PATCH 1/8] interface additions for the C thread implementation Jens Gustedt
2014-08-30 18:46 ` [PATCH 2/8] additions to src/time Jens Gustedt
2014-08-31 0:13 ` Rich Felker
2014-08-31 7:15 ` Jens Gustedt
2014-08-31 12:45 ` Rich Felker
2014-08-30 18:46 ` [PATCH 3/8] use weak symbols for the POSIX functions that will be used by C threads Jens Gustedt
2014-08-31 0:17 ` Rich Felker
2014-08-31 7:18 ` Jens Gustedt
2014-08-30 18:47 ` [PATCH 4/8] add the functions for tss_t and once_flag Jens Gustedt
2014-08-30 18:47 ` [PATCH 5/8] add the functions for mtx_t Jens Gustedt
2014-08-30 18:47 ` [PATCH 6/8] add the functions for cnd_t Jens Gustedt
2014-08-31 0:35 ` Rich Felker
2014-08-31 7:26 ` Jens Gustedt
2014-08-30 18:47 ` [PATCH 7/8] add the thrd_xxxxxx functions Jens Gustedt
2014-08-31 0:46 ` Rich Felker
2014-08-31 7:57 ` Jens Gustedt
2014-08-31 9:51 ` Alexander Monakov
2014-08-31 10:50 ` Jens Gustedt
2014-08-31 11:06 ` Alexander Monakov
2014-08-31 11:31 ` Szabolcs Nagy
2014-08-31 12:57 ` Rich Felker [this message]
2014-08-31 13:19 ` Jens Gustedt
2014-08-31 14:05 ` Rich Felker
2014-08-31 15:07 ` Jens Gustedt
2014-08-31 16:06 ` Rich Felker
2014-08-31 16:36 ` Jens Gustedt
2014-08-31 17:02 ` Rich Felker
2014-08-31 19:10 ` Jens Gustedt
2014-09-01 0:04 ` Rich Felker
2014-08-30 18:47 ` [PATCH 8/8] Separate pthread_create and thrd_create Jens Gustedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140831125745.GW12888@brightrain.aerifal.cx \
--to=dalias@libc.org \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).