[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 5 Jul 2017 12:07:44 -0400
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: move to a proper __lock_t type
On Wed, Jul 05, 2017 at 08:30:14AM +0200, Jens Gustedt wrote:
> Hello,
>
> On Tue, 4 Jul 2017 18:28:42 -0400 Rich Felker <dalias@...c.org> wrote:
>
> > On Wed, Jul 05, 2017 at 01:24:23AM +0300, Alexander Monakov wrote:
> > > On Tue, 4 Jul 2017, Rich Felker wrote:
> > >
> > > > Thanks, applying.
> > >
> > > Can you please document the rationale for using bare ints instead
> > > of an explicit struct for internal locks?
> >
> > I don't think there was/is any good one, it was just the choice that
> > was made at the time. At one point there might have been places it
> > avoided need for including a header to define the type, or where being
> > explicit about the storage needed for the lock mattered (think stdio
> > FILE layout, but it uses its own lock anyway), but I think it was
> > mostly unjustified. I wouldn't be opposed to changing it.
>
> So what would you think of the following migration path:
>
> (1) Finish the volatile/atomic cleanup by adding a_load, a_load_l and
> perhaps corresponding store primitives.
I don't think there are any non-atomic stores to atomic objects; if so
they're either mistakes or they happen synchronized before any
concurrent access, so that the way the store happens does not matter.
Regarding a_load, I see it as a change unrelated to using a lock type;
as long as the object is volatile, direct loads still work fine.
Regarding a_load_l, I'd probably like to avoid it. I'm aiming to phase
out the long/64-bit atomic primitives -- the 64-bit ones are misnomers
as written, even, and there have always been sketchy aliasing issues
going on with their implementations, which I'm not sure we ever fixed.
Right now there are only a few users of them, and if we don't need to
memcpy the signal mask one, there's no reason it needs to even use the
same type/representation as a normal sigset_t array. There's actually
a larger goal to get rid of as much atomic use as possible, outside of
core sync primitive implementations. Many places they're used as
dubious speed or size optimizations where a lock would work perfectly
fine, and would be obviously correct rather than requiring difficult
correctness analysis. The only places that should really be using
atomics outside of implementing sync primitives are places where
there's an AS-safety need that could not be met using locks except
with a huge hammer (sigprocmask around critical section).
> (2) Replace all occurences of "volatile int[2]" by "__lock_t" that
> encapsulates just that in a struct.
>
> (3) Replace the __lock/__unlock pair by the new algorithm
>
> (4) Reduce "__lock_t" to a struct that simply has a "volatile int" as
> sole member.
Can we avoid cosmetics/refactoring like this until the new algorithm
goes in? I just have a backlog of stuff to review and commit, and the
new algorithm is something I really want to get included for release
since it fixes a visible bug (even if hard to hit the race), and
having to go back and start again with refactored versions sets that
back.
> In all of that, would we need the "__" in the name of the structure? I
> think this name will never be exported.
I don't think so, but it might be nice to signify to someone reading
the code that it's not a public type. It might also be mildly nicer in
the debugger if the application also happens to define a type named
lock_t. I'm open to other names too.
Rich
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.
