From: Jens Gustedt <jens.gustedt@inria.fr>
To: musl@lists.openwall.com
Subject: Re: trouble spots for atomic access
Date: Wed, 20 May 2015 09:02:19 +0200 [thread overview]
Message-ID: <1432105339.22607.16.camel@inria.fr> (raw)
In-Reply-To: <20150519231510.GQ17573@brightrain.aerifal.cx>
[-- Attachment #1: Type: text/plain, Size: 4695 bytes --]
Am Dienstag, den 19.05.2015, 19:15 -0400 schrieb Rich Felker:
> On Wed, May 20, 2015 at 12:47:44AM +0200, Jens Gustedt wrote:
> > > > - pthread_once_t should always be volatile
> > > > - pthread_spinlock_t should always be volatile
> > >
> > > These are C++ ABI changes. I'd like to make the change but I'm
> > > concerned it might break things.
> >
> > Both are broken as they are now, if you fall into a compiler that
> > "knows" that the object itself isn't volatile qualified, and by that
> > excuse takes the liberty to optimize out loads. For both types there
> > is one point where there is a cast to (volatile int*) followed by a
> > load, that might not do what we want.
> >
> > (For pthread_once_t, this on line 43 in pthread_once.c)
> >
> > I think the safest would be to make the data types volatile. If that
> > is not possible, do the load with some new function "a_load_rel" that
> > is guaranteed to be volatile, atomic and with memory_order relaxed.
>
> This would require lots of changes and it would be easy to overlook
> some.
No, no, I just meant a special function that is applied for these two
special cases. (With a big warning flag that we only have that because
ABI compatibility issues.)
> The main reason is that lots of the implementations of a_*
> functions perform loads via C code inside their CAS loops and need the
> load to be volatile. They'd all need to be changed to use a_load_rel,
> and a_load_rel would in turn need to be added for all archs.
>
> I have a slightly different approach: destroy the compiler's ability
> to known the identity of the object being accessed by passing it
> through:
>
> volatile int *make_volatile(volatile int *p)
> {
> __asm__ ( "" : "=r"(p) : "0"(p) );
> return p;
> }
Really not so different, I was just thinking to have one line of
assembler that does the load directly. So minor details.
> > > > - pthread_barrier needs atomic increment
> > >
> > > Where?
> >
> > I think all uses of count in pthread_barrier_wait should be
> > atomic. The increments in lines 15 and 93 should be atomic_fetch_add.
> >
> > Probably most archs do a ++ on a volatile as one memory operation, but
> > nothing enforces that.
>
> At line 93, the modification to inst->count happens with a lock held.
> There is no concurrent access.
>
> At line 15, I think there may be an issue; I need to look closer. But
> it's not concurrent writes that need atomicity of the ++ as a RMW
> operation; those are excluded by a lock. It's just the possibility of
> another thread reading concurrently with the writes that I'm concerned
> may happen.
I don't think that both of these are very performance critical, there
are a lot of locks, descheduling etc in that function. So I would
clearly go for security and simplicity here: make them all atomic.
> > > I don't know a really good solution to this. The vast majority of uses
> > > of tid are self->tid, from which perspective tid is not volatile or
> > > even mutable. We don't want to pessimize them with volatile access.
> > > Probably the best approach is to split it out into separate tid and
> > > exit_futex fields.
> >
> > Yes, this would probably be safer and cleaner.
>
> I'm not sure if it's easy to do, though. There seem to be race
> conditions, since ->tid is accessed from other threads for things like
> pthread_kill and pthread_cancel. If we setup ->tid from start(), it
> might not be seen by a pthread_kill or pthread_cancel made by the
> caller of pthread_create after pthread_create returns. If we setup
> ->tid from pthread_create, it would not be seen by the new thread
> immediately when using self->tid. If we setup ->tid from both places,
> then we have a data race (concurrent writes) unless we make it atomic
> (thus volatile) and then we're back to the original problem. (Note
> that we don't have this race now because ->tid is set in the kernel
> before clone returns in either the parent or the child.)
>
> I think the above make_volatile trick would solve the problem without
> adding a new field.
probably
> Alternatively, instead of ->tid and ->exit_futex, we could have ->tid
> (used by self) and ->volatile_tid (used by other threads acting on the
> target).
I would prefer something along that line, looks cleaner to me.
Jens
--
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536 ::
:: :::::::::::::::::::::: gsm France : +33 651400183 ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
next prev parent reply other threads:[~2015-05-20 7:02 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-19 13:57 Jens Gustedt
2015-05-19 22:07 ` Rich Felker
2015-05-19 22:47 ` Jens Gustedt
2015-05-19 23:15 ` Rich Felker
2015-05-20 7:02 ` Jens Gustedt [this message]
2015-05-20 14:55 ` Rich Felker
2015-05-20 11:05 ` Szabolcs Nagy
2015-05-20 14:56 ` Rich Felker
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=1432105339.22607.16.camel@inria.fr \
--to=jens.gustedt@inria.fr \
--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).