Discussion:
libthr shared locks
Konstantin Belousov
2015-12-23 17:25:28 UTC
Permalink
A well-known limitation of the FreeBSD libthr implementation of the
pthread locking objects is the missed support for the process-shared
locks. The hardest part of implementing the support is the neccessity
of providing ABI compatibility for the current implementation.

Right now, the ABI-visible handle to the locks is a single pointer
word. As an example which allows to make the description less vague,
let consider pthread_mutex_t. It is defined in
sys/sys/_pthreadtypes.h as
typedef struct pthread_mutex *pthread_mutex_t;
The pointer points to the following structure, after the
pthread_mutex_init(3) is called
struct pthread_mutex {
struct umutex m_lock;
int m_flags;
struct pthread *m_owner;
int m_count;
int m_spinloops;
int m_yieldloops;
TAILQ_ENTRY(pthread_mutex) m_qe;
};
struct umutex {
volatile __lwpid_t m_owner; /* Owner of the mutex */
__uint32_t m_flags; /* Flags of the mutex */
__uint32_t m_ceilings[2]; /* Priority protect ceiling */
__uint32_t m_spare[4];
};

Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.

Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.

My idea to provide the shared locks, while not changing the ABI for
libthr, is to use marker pointers to indicate the shared objects. The
real struct pthread_mutex, which carries the locking information, is
allocated by at the off-page from some anonymous posix shared memory
object.

The marker is defined as
#define THR_PSHARED_PTR \
((void *)(uintptr_t)((1ULL << (NBBY * sizeof(long) - 1)) | 1))
The bit-pattern is 1000....00001. There are two tricks used:
1. All correctly allocated objects in all supported ABIs are at least
word-aligned, so the least-significant bit cannot be set. This
should made the THR_PSHARED_PTR pattern unique against non-shared
allocations.
2. The high bit is set, which makes the address non-canonical on
amd64, causing attempts to dereference the pointer guaranteed to
segfault, instead of relying of not having the corresponding page
not mapped on the weakly-aligned arches.

The majority of the libthr modifications follow the easy pattern where
the library must store the THR_PSHARED_PTR upon the initialization of
the shared objects, allocate the off-page and initialize the lock
there. If a call assumes that the object is already initialized, then
the we must not instantiate the off-page. To speed-up the lookup, a
cache is kept at the userspace which translates address of locks to
the off-page. Note that we can safely ignore possible unmapping of
the locks, since correct pthread_* API use assumes the call to
pthread_*_destroy() on the end of the object lifecycle. If the lock
is remapped in the usermode, then userspace off-page translation cache
fails, but kernel returns the same shm for lookup, and we end with two
off-page mappings, which is acceptable.

Kernel holds a lookup table which translates the (vm_object, offset)
pair, obtained by the dereference of the user-space address, into the
posix shared memory object. The lifecycle of the shm objects is bound
to the existence of the corresponding vm object.

Note that lifecycle of the kernel objects does not correspond well to
the lifecycle of the vnode vm object. Closed vnode could be recycled
by VFS for whatever reasons, and then we would loose the entry in the
registry. I am not sure if this is very serious issue, since I
suppose that typical use case assumes the anonymous shared memory
backing. Right now kernel drops the off-page shm object on the last
vnode unmap.

Due to backing by the kernel objects, the implementation imposes
per-uid limits on the amount of the shared objects created. An issue
is that there are no such limits in other implementations.

Overhead of the implementation, comparing with the non-process shared
locks, is due to the mandatory off-page lookup, which is mostly
ammortized by the (read-locked) userspace cache. Also, for each
shared lock we get an additional page of memory, which works fine
assuming the applications use limited amount of the shared locks.
Cost for the non-shared locks is a single memory load for each
pthread_* call.

Below are the timing results of my implementation on the 4-core sandy
against the Fedora 22 glibc, done with the same program on the same
hardware (https://www.kib.kiev.ua/kib/pshared/pthread_shared_mtx1.c).

[FreeBSD]
# time /root/pthread_shared_mtx1-64
iter1 10000000 aiter1 10000000 iter2 10000000 aiter2 10000000
./pthread_shared_mtx1-64 2.47s user 3.27s system 166% cpu 3.443 total

[Fedora]
[***@sandy tests]$ /usr/bin/time ./pthread_shared_mtx1-linux64
iter1 10000000 aiter1 10000000 iter2 10000000 aiter2 10000000
1.38user 2.46system 0:01.95elapsed 196%CPU (0avgtext+0avgdata 1576maxresident)k
0inputs+0outputs (0major+142minor)pagefaults 0swaps

The implementation in the patch
https://www.kib.kiev.ua/kib/pshared/pshared.1.patch
gives shared mutexes, condvars, rwlocks and barriers. I did some
smoke-testing, only on amd64. Not implementated are the robust mutexes.
I want to finalize this part of work before implementing robustness,
but some restructuring in the patch, which seems to be arbitrary, like
the normal/pp queues rework to live in arrays, is a preparation to the
robustness feature.

The work was sponsored by The FreeBSD Foundation, previous and current
versions of idea and previous patch were discussed with John Baldwin
and Ed Maste.
Daniel Eischen
2015-12-23 18:03:59 UTC
Permalink
Post by Konstantin Belousov
A well-known limitation of the FreeBSD libthr implementation of the
pthread locking objects is the missed support for the process-shared
locks. The hardest part of implementing the support is the neccessity
of providing ABI compatibility for the current implementation.
Right now, the ABI-visible handle to the locks is a single pointer
word. As an example which allows to make the description less vague,
let consider pthread_mutex_t. It is defined in
sys/sys/_pthreadtypes.h as
typedef struct pthread_mutex *pthread_mutex_t;
The pointer points to the following structure, after the
pthread_mutex_init(3) is called
struct pthread_mutex {
struct umutex m_lock;
int m_flags;
struct pthread *m_owner;
int m_count;
int m_spinloops;
int m_yieldloops;
TAILQ_ENTRY(pthread_mutex) m_qe;
};
struct umutex {
volatile __lwpid_t m_owner; /* Owner of the mutex */
__uint32_t m_flags; /* Flags of the mutex */
__uint32_t m_ceilings[2]; /* Priority protect ceiling */
__uint32_t m_spare[4];
};
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
My idea to provide the shared locks, while not changing the ABI for
libthr, is to use marker pointers to indicate the shared objects. The
real struct pthread_mutex, which carries the locking information, is
allocated by at the off-page from some anonymous posix shared memory
object.
I'd rather just change the pthread_foo lock types to include
the space. I really don't like the way libc, rtld, etc have
to jump through hoops to initialize the locks. If the lock
types included the storage, then they wouldn't have to.

My idea was to keep the first storage unit of the lock as
a pointer/self-reference, so that it is easy to keep the ABI.
You can set the 0 bit in the pointer to indicate whether the
lock was the old ABI, then just clear it before using it.
And we could hide old ABI implementation compilation behind
WITH_FOO to avoid overhead of checking (ptr & 0x1) != 0.
I think this is similar to what you've done below.
Post by Konstantin Belousov
The marker is defined as
#define THR_PSHARED_PTR \
((void *)(uintptr_t)((1ULL << (NBBY * sizeof(long) - 1)) | 1))
[ ... ]

I also don't like to get the kernel involved if it isn't
necessary.
--
DE
Daniel Eischen
2015-12-23 18:27:35 UTC
Permalink
On Wed, 23 Dec 2015, Konstantin Belousov wrote:

[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.

Mixing libraries built with different pthread ABIs should not
be a problem as long as they don't directly operate on the
other's pthread lock types.

There is also our ability to do a library version bump as a last
resort.
--
DE
Konstantin Belousov
2015-12-23 19:05:19 UTC
Permalink
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.

struct my_object {
pthread_mutex_t obj_lock;
...
};

Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.

The issue is similar to the effect of the attempted ino_t expansion
to the 64 bit, which changes the struct stat layout and then triggers
cascade of the ABI issues. With struct stat, most of it is localized in
the src/, which makes handling easier because all of the changed staff
is under our control.

For libpthread, the approach would cause *big* blow up for ports and for
software compiled locally, and we cannot do anything. It would become
the flag day, with subtle bugs all over there after the transition on a
machine where ABIs are mixed. This is not an issue for e.g. appliance
vendors, but IMO is unnaceptable for general-purpose OS.

And, why pay the cost of the flag day for the feature that was not present
up to today ?
Post by Daniel Eischen
Mixing libraries built with different pthread ABIs should not
be a problem as long as they don't directly operate on the
other's pthread lock types.
There is also our ability to do a library version bump as a last
resort.
Bumping libthr version would be not enough. It is very much possible to
get both libthr.so.3 and libthr.so.4 into the same process. Versions
for the symbols must be adjusted to properly bind new and old ABI'
consumers.

I did evaluated this route, and I am between being very skeptical that
it is workable and completely denying the viability. As I pointed out
earlier, this causes enormous amount of bugs due to the ABI change
in third-party, you cannot mix. Clean recompilation for the new ABI
would solve it, but it is not acceptable for FreeBSD pretending to
support upgrades.
Daniel Eischen
2015-12-23 19:48:56 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.
struct my_object {
pthread_mutex_t obj_lock;
...
};
Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.

I think we're just putting off the inevitable. Do we not want
to change our pthread sync types anyway, to get rid of an extra
dereference per lock? To get rid of the hacks in libc, rtld,
etc?

If the answer is no, we never want to do that, then ok.
--
DE
Konstantin Belousov
2015-12-23 20:18:37 UTC
Permalink
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.
struct my_object {
pthread_mutex_t obj_lock;
...
};
Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.
This relegates the issue to an attempt to do the full rebuild. But I
do not see how the port bump would fix it, assume that you are updating
from the 10.x to 11.x and have the mix of the libraries, some of which
were built during the 10.x lifetime but with the bumped ports version.

It is not feasible to do a reliable audit of the 24+ Kports.
Post by Daniel Eischen
I think we're just putting off the inevitable. Do we not want
to change our pthread sync types anyway, to get rid of an extra
dereference per lock? To get rid of the hacks in libc, rtld,
etc?
If the answer is no, we never want to do that, then ok.
An answer to this question requires a) consensus b) a workforce that
would do the decided transition. I evaluated my opinion, potential
consequences and efforts required, and ended up with the posted patch.
Daniel Eischen
2015-12-23 21:30:58 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.
This relegates the issue to an attempt to do the full rebuild. But I
do not see how the port bump would fix it, assume that you are updating
from the 10.x to 11.x and have the mix of the libraries, some of which
were built during the 10.x lifetime but with the bumped ports version.
It is not feasible to do a reliable audit of the 24+ Kports.
Is it really that hard to do a port run and insert a grep
for pthread_{mutex,cond,rwlock}_t in a ports installed
header files? Then just blindly bumping portrevisions
for those ports and those depending on it?

Other than errors caused by storage layouts, libthr can
easily be instrumented to emit a warning when a sync type
is used with the wrong versioned function.
Post by Konstantin Belousov
Post by Daniel Eischen
I think we're just putting off the inevitable. Do we not want
to change our pthread sync types anyway, to get rid of an extra
dereference per lock? To get rid of the hacks in libc, rtld,
etc?
If the answer is no, we never want to do that, then ok.
An answer to this question requires a) consensus b) a workforce that
would do the decided transition. I evaluated my opinion, potential
consequences and efforts required, and ended up with the posted patch.
There's an old patch here:

http://people.freebsd.org/~davidxu/pshared/patch6.diff
--
DE
John Baldwin
2015-12-23 21:22:16 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.
struct my_object {
pthread_mutex_t obj_lock;
...
};
Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.
This relegates the issue to an attempt to do the full rebuild. But I
do not see how the port bump would fix it, assume that you are updating
from the 10.x to 11.x and have the mix of the libraries, some of which
were built during the 10.x lifetime but with the bumped ports version.
It is not feasible to do a reliable audit of the 24+ Kports.
As a bit of a devil's advocate, I think the 64-bit ino_t change will in
fact require this for 11. I suspect 3rd pary apps embed struct stat in
various structures as well and that that ABI change will require not
mixing old and new libraries.

One other point in favor of Konstantin's approach (IMO) is that keeping
the structures private prevents having to maintain the ABI of those
structures in the future. I'm already keenly aware of how painful a
problem that can be with our non-opaque FILE (and which we cannot now
make opaque even though the standard APIs would work fine with an opaque
object).
--
John Baldwin
Konstantin Belousov
2015-12-24 10:45:56 UTC
Permalink
Post by John Baldwin
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.
struct my_object {
pthread_mutex_t obj_lock;
...
};
Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.
This relegates the issue to an attempt to do the full rebuild. But I
do not see how the port bump would fix it, assume that you are updating
from the 10.x to 11.x and have the mix of the libraries, some of which
were built during the 10.x lifetime but with the bumped ports version.
It is not feasible to do a reliable audit of the 24+ Kports.
As a bit of a devil's advocate, I think the 64-bit ino_t change will in
fact require this for 11. I suspect 3rd pary apps embed struct stat in
various structures as well and that that ABI change will require not
mixing old and new libraries.
The stat change is different in this. I do think (and quick overview
of includes seems to support the opinion) that the struct stat is rarely
embedded into the user objects. I already wrote about this, when I said
that 'struct stat' changes are mostly confined to the src/ tree, in one
of the previous messages. When I thought about the ino_t 64bit change,
the only example of affected app that I could remember, without looking
at the code, was rogue/nethack.
Post by John Baldwin
One other point in favor of Konstantin's approach (IMO) is that keeping
the structures private prevents having to maintain the ABI of those
structures in the future. I'm already keenly aware of how painful a
problem that can be with our non-opaque FILE (and which we cannot now
make opaque even though the standard APIs would work fine with an opaque
object).
My patch does not prevent future change to use in-place pthread_mutex_t,
since the patch does not change current ABI. If somebody is willing to
go that route, well, fine. Nobody productized David' work for ten (?)
years. And the reason is, I believe, that the ABI change required to the
fundamental facility (threads did become ubiquios) at that late point in
the OS lifecycle is destructive.

The issue we have after the embedded structures layout modification is
exactly the issue which cannot be handled by symbol versioning.
Daniel Eischen
2015-12-24 13:45:38 UTC
Permalink
Post by John Baldwin
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.
struct my_object {
pthread_mutex_t obj_lock;
...
};
Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.
This relegates the issue to an attempt to do the full rebuild. But I
do not see how the port bump would fix it, assume that you are updating
from the 10.x to 11.x and have the mix of the libraries, some of which
were built during the 10.x lifetime but with the bumped ports version.
It is not feasible to do a reliable audit of the 24+ Kports.
As a bit of a devil's advocate, I think the 64-bit ino_t change will in
fact require this for 11. I suspect 3rd pary apps embed struct stat in
various structures as well and that that ABI change will require not
mixing old and new libraries.
One other point in favor of Konstantin's approach (IMO) is that keeping
the structures private prevents having to maintain the ABI of those
structures in the future. I'm already keenly aware of how painful a
problem that can be with our non-opaque FILE (and which we cannot now
make opaque even though the standard APIs would work fine with an opaque
object).
We would include extra/spare words in the struct from day 1.
The public struct should just consist of an array of storage
units or similar, so that nothing can be gleaned from the
elements of the struct. This is what Solaris does (or at least
used to).

Going forward, I think the sync structures just need to be able
to be properly initialized with PTHREAD_FOO_INITIALIZER. If
10 years out the spare words are not enough, we could still
allocate remaining storage on first use like we do now.
--
DE
John Baldwin
2015-12-24 17:25:33 UTC
Permalink
Post by Daniel Eischen
Post by John Baldwin
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
[ ... ]
Post by Konstantin Belousov
Would the ABI modified to make the pthread_mutex_t large enough to
hold struct pthread_mutex, the rest of the implementation of the
shared mutex is relatively trivial, if not already done.
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
I think this is only if the libraries (or apps) pass pthread
lock types between them, that one has initialized with one ABI
but the other library uses another ABI. We should really
exclude this as part of our ABI compatibility.
Applications commonly embed pthread locks into their objects, including
the exposed ABI in the third-party libraries.
struct my_object {
pthread_mutex_t obj_lock;
...
};
Changing the size of the pthread locks causes uncontrolled breakage of
the ABI for significant number of third-party code.
If the application creates the object itself or allocates storage
for it, basically, if it isn't opaque, yes. But we can bump port
revisions for affected libraries (probably just searching
/usr/local/include/... for pthread_mutex, pthread_cond, etc
types to see possible problems. I did a search for the installed
ports on my system and found a few that might cause problems.
This relegates the issue to an attempt to do the full rebuild. But I
do not see how the port bump would fix it, assume that you are updating
from the 10.x to 11.x and have the mix of the libraries, some of which
were built during the 10.x lifetime but with the bumped ports version.
It is not feasible to do a reliable audit of the 24+ Kports.
As a bit of a devil's advocate, I think the 64-bit ino_t change will in
fact require this for 11. I suspect 3rd pary apps embed struct stat in
various structures as well and that that ABI change will require not
mixing old and new libraries.
One other point in favor of Konstantin's approach (IMO) is that keeping
the structures private prevents having to maintain the ABI of those
structures in the future. I'm already keenly aware of how painful a
problem that can be with our non-opaque FILE (and which we cannot now
make opaque even though the standard APIs would work fine with an opaque
object).
We would include extra/spare words in the struct from day 1.
The public struct should just consist of an array of storage
units or similar, so that nothing can be gleaned from the
elements of the struct. This is what Solaris does (or at least
used to).
Going forward, I think the sync structures just need to be able
to be properly initialized with PTHREAD_FOO_INITIALIZER. If
10 years out the spare words are not enough, we could still
allocate remaining storage on first use like we do now.
You can't allocate extra storage for the PSHARED case. Any changes
to PSHARED primitives that require altering the layout are full-blown
ABI breakages the same as the one being contemplated here.
--
John Baldwin
Daniel Eischen
2015-12-24 18:46:25 UTC
Permalink
Post by John Baldwin
Post by Daniel Eischen
We would include extra/spare words in the struct from day 1.
The public struct should just consist of an array of storage
units or similar, so that nothing can be gleaned from the
elements of the struct. This is what Solaris does (or at least
used to).
Going forward, I think the sync structures just need to be able
to be properly initialized with PTHREAD_FOO_INITIALIZER. If
10 years out the spare words are not enough, we could still
allocate remaining storage on first use like we do now.
You can't allocate extra storage for the PSHARED case. Any changes
to PSHARED primitives that require altering the layout are full-blown
ABI breakages the same as the one being contemplated here.
Yes, I know. With spare slots and being able to move anything required
for shared to near the beginning of the struct, I think we'd be good
for quite a while anyway.

Can we still implement things like robust and priority mutexes in a
pshared mutex with Konstantin's implementation? The rest of the world
(Linux, Solaris anyway) use structs and get by just fine ABI-wise.
--
DE
Konstantin Belousov
2015-12-24 19:14:08 UTC
Permalink
Post by Daniel Eischen
Can we still implement things like robust and priority mutexes in a
pshared mutex with Konstantin's implementation? The rest of the world
(Linux, Solaris anyway) use structs and get by just fine ABI-wise.
Yes, we do. I intend to do this as the next stage, planned the
implementation and did some preparations for it in the current patch.
This raises the question, why do you suppose that my approach with
the off-page would not allow it ? I am asking not to argument, but to
understand a possible shortcoming in the approach, which I missed.

From the implementation PoV, off-page is completely equivalent to the
in-line structs approach, and the difference is only in the initial
translation of say pthread_mutex_t to the structure (off-page lookup vs.
identity). The issues with maintaining the lists of the owned robust of
pi mutexes are same, and in fact you could see some non-trivial traces
of this in the _mutex_fork() reimplementation in my patch.

Linux and Solaris get (large enough) structs early enough to avoid the ABI
issues. I remember Linux changing FILE layout in early days of glibc 2.0
and resulting pain, while they already had the GNU symbol versioning
working and used.
Daniel Eischen
2015-12-25 18:18:14 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
Can we still implement things like robust and priority mutexes in a
pshared mutex with Konstantin's implementation? The rest of the world
(Linux, Solaris anyway) use structs and get by just fine ABI-wise.
Yes, we do. I intend to do this as the next stage, planned the
implementation and did some preparations for it in the current patch.
This raises the question, why do you suppose that my approach with
the off-page would not allow it ? I am asking not to argument, but to
understand a possible shortcoming in the approach, which I missed.
No, I was merely inquiring, if this is on your agenda, that's
good.
Post by Konstantin Belousov
From the implementation PoV, off-page is completely equivalent to the
in-line structs approach, and the difference is only in the initial
translation of say pthread_mutex_t to the structure (off-page lookup vs.
identity). The issues with maintaining the lists of the owned robust of
pi mutexes are same, and in fact you could see some non-trivial traces
of this in the _mutex_fork() reimplementation in my patch.
Linux and Solaris get (large enough) structs early enough to avoid the ABI
issues. I remember Linux changing FILE layout in early days of glibc 2.0
and resulting pain, while they already had the GNU symbol versioning
working and used.
I guess the issue now is the apparent speedups without having the
extra de-reference. 8-10% for things like mysql might be worth
considering the userland structs.
--
DE
Konstantin Belousov
2015-12-26 10:54:09 UTC
Permalink
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
Can we still implement things like robust and priority mutexes in a
pshared mutex with Konstantin's implementation? The rest of the world
(Linux, Solaris anyway) use structs and get by just fine ABI-wise.
Yes, we do. I intend to do this as the next stage, planned the
implementation and did some preparations for it in the current patch.
This raises the question, why do you suppose that my approach with
the off-page would not allow it ? I am asking not to argument, but to
understand a possible shortcoming in the approach, which I missed.
No, I was merely inquiring, if this is on your agenda, that's
good.
Post by Konstantin Belousov
From the implementation PoV, off-page is completely equivalent to the
in-line structs approach, and the difference is only in the initial
translation of say pthread_mutex_t to the structure (off-page lookup vs.
identity). The issues with maintaining the lists of the owned robust of
pi mutexes are same, and in fact you could see some non-trivial traces
of this in the _mutex_fork() reimplementation in my patch.
Linux and Solaris get (large enough) structs early enough to avoid the ABI
issues. I remember Linux changing FILE layout in early days of glibc 2.0
and resulting pain, while they already had the GNU symbol versioning
working and used.
I guess the issue now is the apparent speedups without having the
extra de-reference. 8-10% for things like mysql might be worth
considering the userland structs.
I am sorry for the delay in answering, I read the David' patch you pointed
out, and it took time to get through 9KLOCs patch which is not applicable
to the current sources.

I should note that the patch is not immediately useable, not due to
the source code drift, but because the patch combines many unrelated
things, and I do not agree with everything in it.

E.g., I found quite interesting the notion that our libthr fork() does
not work when called from the signal handler. I do not quite understand
how the detection of the situation is done in the patch, and I do not
agree with the cleanup of the 'in sighandler' state on longjmp. I
think that this part of the patch is obsoleted by libthr intercepting
signals and postponing signal delivery until a critical section is
exited.

Patch adds yet another private malloc(), now to libthr. I think that
cooperating with rtld and share the same allocator would be more
reasonable.

Notes above are to explain why I think that productizing the David' patch
is huge work, IMO significantly more than just 'test and commit'.

Returning to the point of potential gain in performance due to the
ABI change. As I already stated in my previous replies, I am quite
worried about the impact of the ABI change, and I think that potential
destabilizations, which would manifest itself in the subtle and hard to
diagnose ways (i.e. it is not a sigsegv outright on the start) is huge
setback for the change.

OTOH, we should be able to plan such change, which requires much more
drastic measures to be implementable. I started thinking about it, and
I noted that what is needed is solved by renaming libthr to something
else, e.g. libthr2. One of the issue which is not solved by the dso
version bump (not to even mention versioned symbols bump) is the
static linker bringing both libthr.so.3 and libthr.so.4 into the same
process, by the mere use of -lpthread by different objects at different
times. Then, libthr and hypothetical libthr2 should check and prevent
activation of image which loads both.

But these are longtime to evaluate and implement. The feature at hand
does not require ABI change, as my patch demostrated. Yes, shared
mutexes would be more naturally implemented with the inline locks, and
avoidance of an indirection in the libthr is also good, but lets not mix
significant ABI change and some less ambitious feature. My approach does
not lock out future strategies.
Daniel Eischen
2015-12-26 17:15:43 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Daniel Eischen
Can we still implement things like robust and priority mutexes in a
pshared mutex with Konstantin's implementation? The rest of the world
(Linux, Solaris anyway) use structs and get by just fine ABI-wise.
Yes, we do. I intend to do this as the next stage, planned the
implementation and did some preparations for it in the current patch.
This raises the question, why do you suppose that my approach with
the off-page would not allow it ? I am asking not to argument, but to
understand a possible shortcoming in the approach, which I missed.
No, I was merely inquiring, if this is on your agenda, that's
good.
Post by Konstantin Belousov
From the implementation PoV, off-page is completely equivalent to the
in-line structs approach, and the difference is only in the initial
translation of say pthread_mutex_t to the structure (off-page lookup vs.
identity). The issues with maintaining the lists of the owned robust of
pi mutexes are same, and in fact you could see some non-trivial traces
of this in the _mutex_fork() reimplementation in my patch.
Linux and Solaris get (large enough) structs early enough to avoid the ABI
issues. I remember Linux changing FILE layout in early days of glibc 2.0
and resulting pain, while they already had the GNU symbol versioning
working and used.
I guess the issue now is the apparent speedups without having the
extra de-reference. 8-10% for things like mysql might be worth
considering the userland structs.
I am sorry for the delay in answering, I read the David' patch you pointed
out, and it took time to get through 9KLOCs patch which is not applicable
to the current sources.
It is the holidays, I'm sure everyone is busy with other things.
I am in the same camp :-)
Post by Konstantin Belousov
I should note that the patch is not immediately useable, not due to
the source code drift, but because the patch combines many unrelated
things, and I do not agree with everything in it.
Yeah, I would advocate only committing part of David's patch,
in stages. Just the part that implements the struct change and
shared synch primitives. Other changes may no longer apply or
can be done later.
Post by Konstantin Belousov
E.g., I found quite interesting the notion that our libthr fork() does
not work when called from the signal handler. I do not quite understand
how the detection of the situation is done in the patch, and I do not
agree with the cleanup of the 'in sighandler' state on longjmp. I
think that this part of the patch is obsoleted by libthr intercepting
signals and postponing signal delivery until a critical section is
exited.
I think libthr has always done that (postpoining until critical
section is exited). But there are libc (and perhaps other)
locks that may be left locked on a fork(). Really (according
to POSIX), a fork() from a threaded program can only be used
to exec() something.
Post by Konstantin Belousov
Patch adds yet another private malloc(), now to libthr. I think that
cooperating with rtld and share the same allocator would be more
reasonable.
I tend to agree.
Post by Konstantin Belousov
Notes above are to explain why I think that productizing the David' patch
is huge work, IMO significantly more than just 'test and commit'.
Returning to the point of potential gain in performance due to the
ABI change. As I already stated in my previous replies, I am quite
worried about the impact of the ABI change, and I think that potential
destabilizations, which would manifest itself in the subtle and hard to
diagnose ways (i.e. it is not a sigsegv outright on the start) is huge
setback for the change.
I guess I am just not that worried about the ABI change when combined
with a library version bump. We use to do this for every new major
release, it isn't anything that hasn't been done before. We never
supported 2 thread libraries in the same executable, and it is
obvious when that happens (I think we even instrumented libc_r,
or was it rtld?, to emit a warning when that happened. My memory
is fuzzy). With poudriere, our new package system, I would think
we would be better off now than 10 years ago?

I do note that I was initially wrong in thinking that port revision
bumps could help alleviate any pain. A port could be built in
-stable with the revision bump just as well as in -current (or 11).
What would be needed is for pkg to understand that any port built
for 10 and previous could not run on 11+ (just to be on the safe
side).
Post by Konstantin Belousov
OTOH, we should be able to plan such change, which requires much more
drastic measures to be implementable. I started thinking about it, and
I noted that what is needed is solved by renaming libthr to something
else, e.g. libthr2. One of the issue which is not solved by the dso
version bump (not to even mention versioned symbols bump) is the
static linker bringing both libthr.so.3 and libthr.so.4 into the same
process, by the mere use of -lpthread by different objects at different
times. Then, libthr and hypothetical libthr2 should check and prevent
activation of image which loads both.
But these are longtime to evaluate and implement. The feature at hand
does not require ABI change, as my patch demostrated. Yes, shared
mutexes would be more naturally implemented with the inline locks, and
avoidance of an indirection in the libthr is also good, but lets not mix
significant ABI change and some less ambitious feature. My approach does
not lock out future strategies.
I agree, but the work that you are doing now would be basically
thrown out later on. I will not stand in your way and appreciate
any work you do. I would just rather that the struct change be
made now for 11, even without any pshared or other changes. For
once the struct change is made, pshared or other additions can
be made afterward, even in the 11 branch because they would not
break the ABI.
--
DE
Konstantin Belousov
2015-12-26 23:44:24 UTC
Permalink
Post by Daniel Eischen
I think libthr has always done that (postpoining until critical
section is exited). But there are libc (and perhaps other)
locks that may be left locked on a fork(). Really (according
to POSIX), a fork() from a threaded program can only be used
to exec() something.
libthr only started to do the interception in r212076, which was
committed by David on Sep 1, 2010.

Posix left the behaviour undefined after the fork, but real world
rejects implementations which make C runtime broken after fork in mt
process. Rtld must work in the child anyway (and in sighandler as well).
We were forced to ensure also that malloc(3) and pthread_create(3) work
in the child after the fork, and any transient regressions in this area
where immediately reported. One of the biggest abusers of that are
Qt/KDE.
Post by Daniel Eischen
I guess I am just not that worried about the ABI change when combined
with a library version bump. We use to do this for every new major
release, it isn't anything that hasn't been done before. We never
supported 2 thread libraries in the same executable, and it is
obvious when that happens (I think we even instrumented libc_r,
or was it rtld?, to emit a warning when that happened. My memory
is fuzzy). With poudriere, our new package system, I would think
we would be better off now than 10 years ago?
We did significant work to avoid requiring complete recompilation of all
user binaries on the major version updates. Part of it is the symbol
versioning, another part is the stronger discipline and future planning
when doing ABI-sensitive changes. At least starting with stable/8, when
you binary depends only on the basic C runtime (i.e. rtld + libc + libm
+ libthr), you are no longer required to recompile it, and you do not
need to install any compat libraries. Things just work.

We are not there yet, since some libraries are not managed that good,
e.g. libutil is not symver-ed but at least usually bumped correctly.
Some libraries are handled much worse, e.g. libcam _interface_ depends
on volatile kernel parts and libcam version is not always bumped when
needed. Unfortunately, libcam is the dependency of high-profile user
applications, from Gnome and KDE. This is what prevents us from
stopping declaring 'recompile ports on major upgrade', but it is a
goal.

Still, basic C runtime is in much better shape WRT ABI stability
than it was, say, in the 6.x-7.x time. This is why ino_t 64bit is moved
with so much caution, and should also explain why I am so nervous to
lock inlining. Doing libthr bump would break things.
Post by Daniel Eischen
I do note that I was initially wrong in thinking that port revision
bumps could help alleviate any pain. A port could be built in
-stable with the revision bump just as well as in -current (or 11).
What would be needed is for pkg to understand that any port built
for 10 and previous could not run on 11+ (just to be on the safe
side).
BTW, I tried to explain exactly this scenario in one of the previous
replies.
Post by Daniel Eischen
Post by Konstantin Belousov
OTOH, we should be able to plan such change, which requires much more
drastic measures to be implementable. I started thinking about it, and
I noted that what is needed is solved by renaming libthr to something
else, e.g. libthr2. One of the issue which is not solved by the dso
version bump (not to even mention versioned symbols bump) is the
static linker bringing both libthr.so.3 and libthr.so.4 into the same
process, by the mere use of -lpthread by different objects at different
times. Then, libthr and hypothetical libthr2 should check and prevent
activation of image which loads both.
But these are longtime to evaluate and implement. The feature at hand
does not require ABI change, as my patch demostrated. Yes, shared
mutexes would be more naturally implemented with the inline locks, and
avoidance of an indirection in the libthr is also good, but lets not mix
significant ABI change and some less ambitious feature. My approach does
not lock out future strategies.
I agree, but the work that you are doing now would be basically
thrown out later on. I will not stand in your way and appreciate
any work you do. I would just rather that the struct change be
made now for 11, even without any pshared or other changes. For
once the struct change is made, pshared or other additions can
be made afterward, even in the 11 branch because they would not
break the ABI.
Lock inlining was not done for ten years, now cost of doing it is
extremely high, as discussed above. Who would drive the change, and
with what time frame ? If me, I seriosly consider renaming libthr
to libthr2, but I had no time to think much about it.

Right now, I think that I want to commit my current patch and implement
robust mutexes as the next step, without ABI breakage. At least, this
seems to have fixed time-frame and can be made ready for 11.x. Lock
inlining might be not. Are there serious objections against the plan,
except that (lock inlining + pshared) is ideal situation, while the plan
is not (but more practical) ?
Daniel Eischen
2015-12-27 16:44:44 UTC
Permalink
[ snipped for brevity ]
Post by Konstantin Belousov
Post by Daniel Eischen
I agree, but the work that you are doing now would be basically
thrown out later on. I will not stand in your way and appreciate
any work you do. I would just rather that the struct change be
made now for 11, even without any pshared or other changes. For
once the struct change is made, pshared or other additions can
be made afterward, even in the 11 branch because they would not
break the ABI.
Lock inlining was not done for ten years, now cost of doing it is
extremely high, as discussed above. Who would drive the change, and
with what time frame ? If me, I seriosly consider renaming libthr
to libthr2, but I had no time to think much about it.
I could probably do the inlining of locks, pulling out that part
of it from David's patch, and coordinating with you so we don't
step on each other's toes. Perhaps just making the first element
of the structs a self-reference at first, would help mitigate
that...

I'm not sure what renaming libthr to libthr2 solves that a
version bump can't also. Can't we still tell whether both
libthr.so.3 and libthr.so.4 have been loaded? Perhaps libthr2
is cleaner WRT keeping the old ABI (it could be dropped?).
Post by Konstantin Belousov
Right now, I think that I want to commit my current patch and implement
robust mutexes as the next step, without ABI breakage. At least, this
seems to have fixed time-frame and can be made ready for 11.x. Lock
inlining might be not. Are there serious objections against the plan,
except that (lock inlining + pshared) is ideal situation, while the plan
is not (but more practical) ?
What is the timeframe for 11.0-RELEASE? If not for 11.0, I would
like to see it done soon in the 12-current branch afterward.
In my mind, any pain will be the same in 11 or 12, nothing really
is gained by waiting, only by not doing it (inlining locks) ever.
--
DE
Konstantin Belousov
2015-12-28 10:51:57 UTC
Permalink
Post by Daniel Eischen
[ snipped for brevity ]
Post by Konstantin Belousov
Post by Daniel Eischen
I agree, but the work that you are doing now would be basically
thrown out later on. I will not stand in your way and appreciate
any work you do. I would just rather that the struct change be
made now for 11, even without any pshared or other changes. For
once the struct change is made, pshared or other additions can
be made afterward, even in the 11 branch because they would not
break the ABI.
Lock inlining was not done for ten years, now cost of doing it is
extremely high, as discussed above. Who would drive the change, and
with what time frame ? If me, I seriosly consider renaming libthr
to libthr2, but I had no time to think much about it.
I could probably do the inlining of locks, pulling out that part
of it from David's patch, and coordinating with you so we don't
step on each other's toes. Perhaps just making the first element
of the structs a self-reference at first, would help mitigate
that...
Adding the self-pointer is good idea, but it would not work for the
shared locks. If you are going to work on this, then I will scratch
my patch, to not impede on your work.

Taking out the inlining bits from the David patch, or (which would I do,
if doing this) just reimplementing it from scratch is easy enough and
just require some time. I estimated this job to take between one and
two weeks.

What is really thrilling is to manage the consequences of the ABI
breakage. When I did the evaluation for the FF project, I put a six
months extent for the whole work. This is for things like looking at
the ports impact, trying to know in advance where mixed things start to
break, monitoring the users complains about issues caused by the ABI
break etc.

We would see afterward if I overestimated the work, but I do the typical
mistake of underestimating usually, whatever insane large the initial
numbers are.
Post by Daniel Eischen
I'm not sure what renaming libthr to libthr2 solves that a
version bump can't also. Can't we still tell whether both
libthr.so.3 and libthr.so.4 have been loaded? Perhaps libthr2
is cleaner WRT keeping the old ABI (it could be dropped?).
Yes, part of the change probably should be a prevention of simultaneous
existence of libthr.so.3 and (libthr.so.4 or libthr2.so.1, whatever
it is named) in one process. This should be done either with rtld
facilities, I am not sure how. Or e.g. with dlopen("otherlib",
RTLD_NOLOAD) in the constructor of each library and failing if dlopen()
returns a valid handle.

Another immediate point, not only libthr.so must be bumped, but also all
base libraries depending on it must be. Even if some libraries do not
directly record the dependency, they might require the bump as well, I
am thinking about c++ runtime. This should allow the compat packages
to provide useable libraries.

IMO this is easier to see when the libraries names differ significantly
in the name part, and not in the .so.n part. At least users would be
less surprised, but also the work of the person who tracks the deps,
would be easier too.
Post by Daniel Eischen
Post by Konstantin Belousov
Right now, I think that I want to commit my current patch and implement
robust mutexes as the next step, without ABI breakage. At least, this
seems to have fixed time-frame and can be made ready for 11.x. Lock
inlining might be not. Are there serious objections against the plan,
except that (lock inlining + pshared) is ideal situation, while the plan
is not (but more practical) ?
What is the timeframe for 11.0-RELEASE? If not for 11.0, I would
like to see it done soon in the 12-current branch afterward.
In my mind, any pain will be the same in 11 or 12, nothing really
is gained by waiting, only by not doing it (inlining locks) ever.
Difference is in causing more (ABI) breakage on the near-coming
stable/11 users, or trying to fix or mitigate more of it in HEAD,
using the local population as the canaries.

You see my position, I would avoid ABI breakage at all costs. If I
cannot talk you against this, please do not consider the work done after
the inlining patch is committed to HEAD, it is actually only start
there.
Daniel Eischen
2015-12-28 16:59:02 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
[ snipped for brevity ]
Post by Konstantin Belousov
Post by Daniel Eischen
I agree, but the work that you are doing now would be basically
thrown out later on. I will not stand in your way and appreciate
any work you do. I would just rather that the struct change be
made now for 11, even without any pshared or other changes. For
once the struct change is made, pshared or other additions can
be made afterward, even in the 11 branch because they would not
break the ABI.
Lock inlining was not done for ten years, now cost of doing it is
extremely high, as discussed above. Who would drive the change, and
with what time frame ? If me, I seriosly consider renaming libthr
to libthr2, but I had no time to think much about it.
I could probably do the inlining of locks, pulling out that part
of it from David's patch, and coordinating with you so we don't
step on each other's toes. Perhaps just making the first element
of the structs a self-reference at first, would help mitigate
that...
Adding the self-pointer is good idea, but it would not work for the
shared locks. If you are going to work on this, then I will scratch
my patch, to not impede on your work.
Shared locks would work, but only for the new ABI (I guess that
would be FBSD1.4 in HEAD, or FBSD1.5 if we bumped it again just
for this).

Another option is to just increase the size for the pthread synch
types without really changing the first element (still a pointer
to the allocated object - the implementation would stay the same).
The only breakage would then be storage layout issues for new
compiles linking against older ones. For FreeBSD-12, we could
change the implementation to a real inlining (without changing
the storage size), that gives a major release cycle for 3rd
parties to make the change. I'm not sure if this would really
be much of a benefit - the storage size change alone would
probably be the greatest pain.
Post by Konstantin Belousov
Taking out the inlining bits from the David patch, or (which would I do,
if doing this) just reimplementing it from scratch is easy enough and
just require some time. I estimated this job to take between one and
two weeks.
I think a lot of David's patch is the renaming of all the
elements of the public structs to prepend '__'. I was thinking
it would be nice to have the public structs be something like
this:

struct pthread_mutex_t {
uint32_t __x[IMPL_REQ + IMPL_SPARE + pad_to_CACHE_LINE_SIZE];
};

and then have libthr override the definition. That would
make declaring PTHREAD_MUTEX_INITIALIZER, etc, a little
magical, but avoid a lot of needless churn in libthr.

But, I agree, just doing the inlining is basic grunt work
and limited in duration. Your estimate seems reasonable.
Post by Konstantin Belousov
What is really thrilling is to manage the consequences of the ABI
breakage. When I did the evaluation for the FF project, I put a six
months extent for the whole work. This is for things like looking at
the ports impact, trying to know in advance where mixed things start to
break, monitoring the users complains about issues caused by the ABI
break etc.
There's not much we can really do to manage it, just make it
known that all ports need to be recompiled on 11 in order to
be on the safe side. The libmap.conf trick will not work
either.

If we could instrument libthr or rtld to emit a warning/error
message with a URL to a FAQ or the the src/UPDATING entry that
would be nice.
Post by Konstantin Belousov
We would see afterward if I overestimated the work, but I do the typical
mistake of underestimating usually, whatever insane large the initial
numbers are.
Post by Daniel Eischen
I'm not sure what renaming libthr to libthr2 solves that a
version bump can't also. Can't we still tell whether both
libthr.so.3 and libthr.so.4 have been loaded? Perhaps libthr2
is cleaner WRT keeping the old ABI (it could be dropped?).
Yes, part of the change probably should be a prevention of simultaneous
existence of libthr.so.3 and (libthr.so.4 or libthr2.so.1, whatever
it is named) in one process. This should be done either with rtld
facilities, I am not sure how. Or e.g. with dlopen("otherlib",
RTLD_NOLOAD) in the constructor of each library and failing if dlopen()
returns a valid handle.
Another immediate point, not only libthr.so must be bumped, but also all
base libraries depending on it must be. Even if some libraries do not
directly record the dependency, they might require the bump as well, I
am thinking about c++ runtime. This should allow the compat packages
to provide useable libraries.
IMO this is easier to see when the libraries names differ significantly
in the name part, and not in the .so.n part. At least users would be
less surprised, but also the work of the person who tracks the deps,
would be easier too.
Post by Daniel Eischen
Post by Konstantin Belousov
Right now, I think that I want to commit my current patch and implement
robust mutexes as the next step, without ABI breakage. At least, this
seems to have fixed time-frame and can be made ready for 11.x. Lock
inlining might be not. Are there serious objections against the plan,
except that (lock inlining + pshared) is ideal situation, while the plan
is not (but more practical) ?
What is the timeframe for 11.0-RELEASE? If not for 11.0, I would
like to see it done soon in the 12-current branch afterward.
In my mind, any pain will be the same in 11 or 12, nothing really
is gained by waiting, only by not doing it (inlining locks) ever.
Difference is in causing more (ABI) breakage on the near-coming
stable/11 users, or trying to fix or mitigate more of it in HEAD,
using the local population as the canaries.
You see my position, I would avoid ABI breakage at all costs. If I
cannot talk you against this, please do not consider the work done after
the inlining patch is committed to HEAD, it is actually only start
there.
Ok, I do think we want to do it at some point. Perhaps 12-current
would be better...
--
DE
Konstantin Belousov
2015-12-29 18:44:05 UTC
Permalink
Post by Daniel Eischen
Post by Konstantin Belousov
Adding the self-pointer is good idea, but it would not work for the
shared locks. If you are going to work on this, then I will scratch
my patch, to not impede on your work.
Shared locks would work, but only for the new ABI (I guess that
would be FBSD1.4 in HEAD, or FBSD1.5 if we bumped it again just
for this).
I only mean that self-pointer sometimes would cause additional issues.
Post by Daniel Eischen
Another option is to just increase the size for the pthread synch
types without really changing the first element (still a pointer
to the allocated object - the implementation would stay the same).
The only breakage would then be storage layout issues for new
compiles linking against older ones. For FreeBSD-12, we could
change the implementation to a real inlining (without changing
the storage size), that gives a major release cycle for 3rd
parties to make the change. I'm not sure if this would really
be much of a benefit - the storage size change alone would
probably be the greatest pain.
It is a good intermediate point in the whole plan of changing the ABI.
The work required to support pshared after the size is increased is not
completely trivial, but is not a rocket science either. But your note
about intermediate step might give additional freedom in executing the
whole plan.
Post by Daniel Eischen
Post by Konstantin Belousov
Taking out the inlining bits from the David patch, or (which would I do,
if doing this) just reimplementing it from scratch is easy enough and
just require some time. I estimated this job to take between one and
two weeks.
I think a lot of David's patch is the renaming of all the
elements of the public structs to prepend '__'. I was thinking
it would be nice to have the public structs be something like
struct pthread_mutex_t {
uint32_t __x[IMPL_REQ + IMPL_SPARE + pad_to_CACHE_LINE_SIZE];
};
and then have libthr override the definition. That would
make declaring PTHREAD_MUTEX_INITIALIZER, etc, a little
magical, but avoid a lot of needless churn in libthr.
This is very good suggestion, I fully agree. There are some more details,
e.g. it would be better to use uint64_t or explicit align attribute, to
get proper alignment, but overall idea is sound, of course.
Post by Daniel Eischen
But, I agree, just doing the inlining is basic grunt work
and limited in duration. Your estimate seems reasonable.
Post by Konstantin Belousov
What is really thrilling is to manage the consequences of the ABI
breakage. When I did the evaluation for the FF project, I put a six
months extent for the whole work. This is for things like looking at
the ports impact, trying to know in advance where mixed things start to
break, monitoring the users complains about issues caused by the ABI
break etc.
There's not much we can really do to manage it, just make it
known that all ports need to be recompiled on 11 in order to
be on the safe side. The libmap.conf trick will not work
either.
No. This is probably the point where our positions diverge most.

I do think that some mitigation measures can be applied, e.g. your
self-pointer trick, check for presence of 'other' lib when loading
given threading library, renaming etc. It should be thought out in
advance.

Another thing to do is to monitor mailing lists, react to the user
issues, see if it is related to the ABI breakage, and possibly create
another measures which would make some issues less painful.

I consider both activities absolutely vital when doing this surgery.
In principle, I may do both, but it requires more planning and is
not immediately reachable comparing with the off-page approach to
only get pshared right now.
Post by Daniel Eischen
Post by Konstantin Belousov
You see my position, I would avoid ABI breakage at all costs. If I
cannot talk you against this, please do not consider the work done after
the inlining patch is committed to HEAD, it is actually only start
there.
Ok, I do think we want to do it at some point. Perhaps 12-current
would be better...
Some people in private also told me that they consider 11 too risky target
for this change. We will see.
Daniel Eischen
2015-12-29 19:18:27 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Adding the self-pointer is good idea, but it would not work for the
shared locks. If you are going to work on this, then I will scratch
my patch, to not impede on your work.
Shared locks would work, but only for the new ABI (I guess that
would be FBSD1.4 in HEAD, or FBSD1.5 if we bumped it again just
for this).
I only mean that self-pointer sometimes would cause additional issues.
Post by Daniel Eischen
Another option is to just increase the size for the pthread synch
types without really changing the first element (still a pointer
to the allocated object - the implementation would stay the same).
The only breakage would then be storage layout issues for new
compiles linking against older ones. For FreeBSD-12, we could
change the implementation to a real inlining (without changing
the storage size), that gives a major release cycle for 3rd
parties to make the change. I'm not sure if this would really
be much of a benefit - the storage size change alone would
probably be the greatest pain.
It is a good intermediate point in the whole plan of changing the ABI.
The work required to support pshared after the size is increased is not
completely trivial, but is not a rocket science either. But your note
about intermediate step might give additional freedom in executing the
whole plan.
Post by Daniel Eischen
Post by Konstantin Belousov
Taking out the inlining bits from the David patch, or (which would I do,
if doing this) just reimplementing it from scratch is easy enough and
just require some time. I estimated this job to take between one and
two weeks.
I think a lot of David's patch is the renaming of all the
elements of the public structs to prepend '__'. I was thinking
it would be nice to have the public structs be something like
struct pthread_mutex_t {
uint32_t __x[IMPL_REQ + IMPL_SPARE + pad_to_CACHE_LINE_SIZE];
};
and then have libthr override the definition. That would
make declaring PTHREAD_MUTEX_INITIALIZER, etc, a little
magical, but avoid a lot of needless churn in libthr.
This is very good suggestion, I fully agree. There are some more details,
e.g. it would be better to use uint64_t or explicit align attribute, to
get proper alignment, but overall idea is sound, of course.
Post by Daniel Eischen
But, I agree, just doing the inlining is basic grunt work
and limited in duration. Your estimate seems reasonable.
Post by Konstantin Belousov
What is really thrilling is to manage the consequences of the ABI
breakage. When I did the evaluation for the FF project, I put a six
months extent for the whole work. This is for things like looking at
the ports impact, trying to know in advance where mixed things start to
break, monitoring the users complains about issues caused by the ABI
break etc.
There's not much we can really do to manage it, just make it
known that all ports need to be recompiled on 11 in order to
be on the safe side. The libmap.conf trick will not work
either.
No. This is probably the point where our positions diverge most.
I do think that some mitigation measures can be applied, e.g. your
self-pointer trick, check for presence of 'other' lib when loading
given threading library, renaming etc. It should be thought out in
advance.
Another thing to do is to monitor mailing lists, react to the user
issues, see if it is related to the ABI breakage, and possibly create
another measures which would make some issues less painful.
I consider both activities absolutely vital when doing this surgery.
In principle, I may do both, but it requires more planning and is
not immediately reachable comparing with the off-page approach to
only get pshared right now.
Post by Daniel Eischen
Post by Konstantin Belousov
You see my position, I would avoid ABI breakage at all costs. If I
cannot talk you against this, please do not consider the work done after
the inlining patch is committed to HEAD, it is actually only start
there.
Ok, I do think we want to do it at some point. Perhaps 12-current
would be better...
Some people in private also told me that they consider 11 too risky target
for this change. We will see.
Perhaps another option is doing all of what we said above
and also use your suggestion of libthr2, but have both libthr
and libthr2, where libthr2 is labeled experimental or something
similar. Have a WITH_LIBTHR2 build flag that conditionally
inlines the pthread locks and enables (links w/-pthread) libthr2
for FreeBSD base OS. A note/comment for the WITH_LIBTHR2 flag
states that all ports must also be recompiled to be on the
safe side. This allows one to switch back and forth, as long
as any affected ports are also rebuilt.

If we were to introduce this in a future 12-current, we might
have the WITH_LIBTHR_FOO flag anyway, at least initially, so
perhaps it wouldn't be extra work.
--
DE
Konstantin Belousov
2015-12-30 14:15:10 UTC
Permalink
Post by Daniel Eischen
Perhaps another option is doing all of what we said above
and also use your suggestion of libthr2, but have both libthr
and libthr2, where libthr2 is labeled experimental or something
similar. Have a WITH_LIBTHR2 build flag that conditionally
inlines the pthread locks and enables (links w/-pthread) libthr2
for FreeBSD base OS. A note/comment for the WITH_LIBTHR2 flag
states that all ports must also be recompiled to be on the
safe side. This allows one to switch back and forth, as long
as any affected ports are also rebuilt.
If we were to introduce this in a future 12-current, we might
have the WITH_LIBTHR_FOO flag anyway, at least initially, so
perhaps it wouldn't be extra work.
This is cheating, but I agree that this is quite plausible approach. I
think about also adding the __INLINE_PTHREAD_LOCKS preprocessor macro to
select inlined locks at the compilation time. In other words, the user
which wants to start experimenting with the stuff would do

cc -D__INLINE_PTHREAD_LOCKS -c ....
cc -o program ... -lpthread2

I may just do this after I finish with my patches.
Brooks Davis
2016-01-11 19:06:25 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Taking out the inlining bits from the David patch, or (which would I do,
if doing this) just reimplementing it from scratch is easy enough and
just require some time. I estimated this job to take between one and
two weeks.
I think a lot of David's patch is the renaming of all the
elements of the public structs to prepend '__'. I was thinking
it would be nice to have the public structs be something like
struct pthread_mutex_t {
uint32_t __x[IMPL_REQ + IMPL_SPARE + pad_to_CACHE_LINE_SIZE];
};
and then have libthr override the definition. That would
make declaring PTHREAD_MUTEX_INITIALIZER, etc, a little
magical, but avoid a lot of needless churn in libthr.
This is very good suggestion, I fully agree. There are some more details,
e.g. it would be better to use uint64_t or explicit align attribute, to
get proper alignment, but overall idea is sound, of course.
Based on our experiences with CHERI I'd suggest that the alignment of
opaque types be at least 128-bit to provide some future proofing. Even
with plenty of spare space (i.e. the giant jmpbuf on MIPS) incresing
alignemnt later adds complexity and opportunities for subtle breakage.

-- Brooks
Daniel Eischen
2015-12-24 14:29:49 UTC
Permalink
[ much snipped for brevity ]
Post by John Baldwin
Post by Konstantin Belousov
It is not feasible to do a reliable audit of the 24+ Kports.
As a bit of a devil's advocate, I think the 64-bit ino_t change will in
fact require this for 11. I suspect 3rd pary apps embed struct stat in
various structures as well and that that ABI change will require not
mixing old and new libraries.
One other point in favor of Konstantin's approach (IMO) is that keeping
the structures private prevents having to maintain the ABI of those
structures in the future. I'm already keenly aware of how painful a
problem that can be with our non-opaque FILE (and which we cannot now
make opaque even though the standard APIs would work fine with an opaque
object).
This seems to be David's latest patch:

http://people.freebsd.org/~davidxu/patch/pshared0607.diff

It is only 3 years old (2012). I have email from David that says
he got 8-10% speedup in mysql OLTP from making the synch types
structures.

The patch also implements robust and priority inheritence mutexes
bumps shared library versions.
--
DE
Julian Elischer
2015-12-26 02:46:45 UTC
Permalink
Post by Konstantin Belousov
A well-known limitation of the FreeBSD libthr implementation of the
pthread locking objects is the missed support for the process-shared
locks. The hardest part of implementing the support is the neccessity
of providing ABI compatibility for the current implementation.
[...]
Changing this ABI is very hard. libthr provides the symbol
versioning, which allows to provide compatible shims for the previous
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
which is one reason I think symbol versioning is over-rated..
just make a new library version.
Simon J. Gerraty
2015-12-26 20:04:10 UTC
Permalink
Post by Julian Elischer
Post by Konstantin Belousov
ABI variant. But since userspace tends to use the pthread objects in
the layouts of the library objects, this causes serious ABI issues
when mixing libraries built against different default versions of
libthr.
which is one reason I think symbol versioning is over-rated..
symbol versioning works fine - provided you have opaque structs.
Post by Julian Elischer
just make a new library version.
as previously noted, this rarely solves anything since to avoid problems
you need to bump the major version of all shared libs, and you cannot do
that for 3rd party code you don't even know exists.
Eric van Gyzen
2016-02-12 23:24:40 UTC
Permalink
Post by Konstantin Belousov
The implementation in the patch
https://www.kib.kiev.ua/kib/pshared/pshared.1.patch
gives shared mutexes, condvars, rwlocks and barriers.
I reviewed everything except kern_umtx.c, which I plan to review on
Monday. Here are my comments so far.

* thr_mutex.c

Thank you for converting some macros to functions. I find functions
much cleaner and easier to read and debug.

* thr_mutex.c line 116

The parentheses around (m) can be removed now.

* thr_mutex.c lines 331-333

m->m_qe.tqe_prev =
TAILQ_NEXT(last_priv, m_qe)->
m_qe.tqe_prev;

This seems to read the m_qe.tqe_prev field from a shared mutex. Is that
safe? It seems like a race. The following would seem more direct,
avoiding the shared mutex:

m->m_qe.tqe_prev = &TAILQ_NEXT(last_prev, m_qe);

* thr_mutex.c line 354

*(q)->tqh_last = last_priv;

This seems to modify the tqe_next field in a shared mutex. Is that
safe? Furthermore, that mutex was/is the last on the list, but we seem
to set its tqe_next pointer to an earlier element, creating a cycle in
the list.

* thr_mutex.c line 451

__pthread_mutex_trylock() calls __thr_pshared_offpage() twice [for
pshared mutexes]. You could eliminate one call by moving
mutex_trylock_common() inline.

* thr_pshared.c line 165

res = NULL seems unnecessary.

* thr_pshared.c

In __thr_pshared_offpage(), can pshared_lookup() fail in the !doalloc
case? pshared_hash seems to be an authority, not just an optimization.
I ask so that I can understand the code and more effectively review it.
In particular, if pshared_lookup() fails and UMTX_SHM_LOOKUP succeeds,
is it possible to get multiple mappings for the same shm object?

* thr_barrier.c line 110

if (bar == NULL)
return (EFAULT);

POSIX does not mention EFAULT. Should we return ENOMEM, or can we
"extend" the standard? (Ditto for all other _init functions.)

* thr_cond.c line 106

You could use cattr instead of the ?: expression.

* thr_rwlock.c

rwlock_init() assumes that __thr_pshared_offpage() does not fail.
Konstantin Belousov
2016-02-13 14:38:15 UTC
Permalink
Thank you for the review.
Post by Eric van Gyzen
* thr_mutex.c line 116
The parentheses around (m) can be removed now.
Done.
Post by Eric van Gyzen
* thr_mutex.c lines 331-333
m->m_qe.tqe_prev =
TAILQ_NEXT(last_priv, m_qe)->
m_qe.tqe_prev;
This seems to read the m_qe.tqe_prev field from a shared mutex. Is that
safe? It seems like a race. The following would seem more direct,
m->m_qe.tqe_prev = &TAILQ_NEXT(last_prev, m_qe);
This is indeed racy, relying on the parent process not unlocking the
shared mutexes. But after your note, I think that the whole list
iteration is unsafe, because of the same unlocking in parent.

So in fact I have to return to what I did in the previous version of the
patch, where I kept two queues for each type of the mutexes, one total,
and one private. The private queue keeps the order of the total, so that
reinitialization of the total queue on the fork is correct for ceiling
ordering.
Post by Eric van Gyzen
* thr_mutex.c line 354
*(q)->tqh_last = last_priv;
This seems to modify the tqe_next field in a shared mutex. Is that
safe? Furthermore, that mutex was/is the last on the list, but we seem
to set its tqe_next pointer to an earlier element, creating a cycle in
the list.
This code is gone due to the previous note.
Post by Eric van Gyzen
* thr_mutex.c line 451
__pthread_mutex_trylock() calls __thr_pshared_offpage() twice [for
pshared mutexes]. You could eliminate one call by moving
mutex_trylock_common() inline.
I see. In fact, I really wanted to eliminate the CHECK_AND_INIT_MUTEX.
See my attempt in the updated patch.
Post by Eric van Gyzen
* thr_pshared.c line 165
res = NULL seems unnecessary.
Done.
Post by Eric van Gyzen
* thr_pshared.c
In __thr_pshared_offpage(), can pshared_lookup() fail in the !doalloc
case? pshared_hash seems to be an authority, not just an optimization.
I ask so that I can understand the code and more effectively review it.
In particular, if pshared_lookup() fails and UMTX_SHM_LOOKUP succeeds,
is it possible to get multiple mappings for the same shm object?
Do you mean, is it possible (and if yes, is it harmful) to have several
virtual addresses in one process for the same key ?

I think there is a bug in pshared_insert() where I preferred new val
over the hashed val (mapping address). In the situation where there are
two threads trying to lock the same object, it may cause first thread to
operate on unmapped address. The scenario is:
- both threads do not find the key in hash;
- first thread performs pshared_insert() and returns the address;
- second thread performs pshared_insert() and replaces the address
in hash, also invalidating the address still used by first thread.
I changed the pshared_insert() to keep the existing hash value, and
unmap the new val.

That said, I think it is probably not very harmful to have different
callers to operate on different mappings for the same key (of course,
the backing page must be shared). I can only think of the problems
due to locked mutex list manipulation functions failing if the address
of element changed. I said that this should not be very harmful since
I suspect that only list invariants checks would fail, and not actual
removal, but I did not checked it.

For now, I think that the invariant I have to ensure is that calls to
lock and unlock from the same thread for the same key get the same
offpage virtual address.
Post by Eric van Gyzen
* thr_barrier.c line 110
if (bar == NULL)
return (EFAULT);
POSIX does not mention EFAULT. Should we return ENOMEM, or can we
"extend" the standard? (Ditto for all other _init functions.)
I think EFAULT is permitted by the 'undefined behaviour' clause, since
the primary cause for the offpage allocation failure is wrong address
passed to the __thr_pshared_offpage(). In other words, if an implementation
did not used offpage, but directly write something to the *m memory, it
would get SIGSEGV.

As noted above, malloc() failure would also lead to EFAULT (and this is
what probably caused your question), but I would consider this improbable,
while invalid address passed to the init function a more likely cause.
I do not want to complicate code to distinguish the cases.
Post by Eric van Gyzen
* thr_cond.c line 106
You could use cattr instead of the ?: expression.
Done.
Post by Eric van Gyzen
* thr_rwlock.c
rwlock_init() assumes that __thr_pshared_offpage() does not fail.
Done, I return EFAULT in case offpage allocation fails. If you object
still against other EFAULTs, I would change all of them to be consistent.

Updated patch is at
https://www.kib.kiev.ua/kib/pshared/pshared.2.patch
Martin Simmons
2016-02-15 14:17:20 UTC
Permalink
Is pthread_barrier_destroy making the wrong comparison?

+ if (barrier == THR_PSHARED_PTR) {

I think this should be *barrier.

Also, a general question: why not use some flag in the barrier (and other
objects) to indicate pshared, removing the need for __thr_pshared_offpage
except in init?

__Martin
Konstantin Belousov
2016-02-15 14:44:10 UTC
Permalink
Post by Martin Simmons
Is pthread_barrier_destroy making the wrong comparison?
+ if (barrier == THR_PSHARED_PTR) {
I think this should be *barrier.
You are right, thank you for noticing.
I uploaded https://www.kib.kiev.ua/kib/pshared/pshared.3.patch
Post by Martin Simmons
Also, a general question: why not use some flag in the barrier (and other
objects) to indicate pshared, removing the need for __thr_pshared_offpage
except in init?
But where would I keep the object ? All that I have with the current
ABI is a single pointer, which de facto behaves like the flag which you
proposed. It is either real pointer or (if set to some specific value
impossible for a valid pointer) there is an offpage.
Martin Simmons
2016-02-15 17:35:33 UTC
Permalink
Post by Konstantin Belousov
Post by Martin Simmons
Also, a general question: why not use some flag in the barrier (and other
objects) to indicate pshared, removing the need for __thr_pshared_offpage
except in init?
But where would I keep the object ? All that I have with the current
ABI is a single pointer, which de facto behaves like the flag which you
proposed. It is either real pointer or (if set to some specific value
impossible for a valid pointer) there is an offpage.
I'm probably missing something, but I was thinking pthread_barrier_init would
do something like

if ( attr is PTHREAD_PROCESS_PRIVATE ) {
bar = calloc(1, sizeof(struct pthread_barrier));
pshared = 0;
} else {
bar = __thr_pshared_offpage(barrier, 1);
pshared = 1;
}
bar->psharedflag = pshared;
*barrier = bar;

Then pthread_barrier_destroy would use the psharedflag slot to decide how to
free it and pthread_barrier_wait would need no changes.
Konstantin Belousov
2016-02-15 17:56:21 UTC
Permalink
Post by Martin Simmons
Post by Konstantin Belousov
Post by Martin Simmons
Also, a general question: why not use some flag in the barrier (and other
objects) to indicate pshared, removing the need for __thr_pshared_offpage
except in init?
But where would I keep the object ? All that I have with the current
ABI is a single pointer, which de facto behaves like the flag which you
proposed. It is either real pointer or (if set to some specific value
impossible for a valid pointer) there is an offpage.
I'm probably missing something, but I was thinking pthread_barrier_init would
do something like
if ( attr is PTHREAD_PROCESS_PRIVATE ) {
bar = calloc(1, sizeof(struct pthread_barrier));
pshared = 0;
} else {
bar = __thr_pshared_offpage(barrier, 1);
pshared = 1;
}
bar->psharedflag = pshared;
*barrier = bar;
Then pthread_barrier_destroy would use the psharedflag slot to decide how to
free it and pthread_barrier_wait would need no changes.
A process which has the page where the initialized pthread_barrier_t is
located, must be able to operate on the barrier. Now, look at your scheme.

One process which executed pthread_barrier_init(), performed what you
proposed. What should do the pthread_barrier_wait() call in another
process, which shares the 'barrier' with the first process, but does
not share the whole address space ? After your pthread_barrier_init()
executed, barrier contains the address of the object (off-page) in the
other address space, for that process.
Martin Simmons
2016-02-16 16:17:46 UTC
Permalink
Post by Konstantin Belousov
One process which executed pthread_barrier_init(), performed what you
proposed. What should do the pthread_barrier_wait() call in another
process, which shares the 'barrier' with the first process, but does
not share the whole address space ? After your pthread_barrier_init()
executed, barrier contains the address of the object (off-page) in the
other address space, for that process.
Ah, sorry, I understand now (the init functions are called before any
sharing).

How should the destroy functions be used by the processes? I.e. should only
the "last" process call destroy or can every process call it?
Konstantin Belousov
2016-02-17 16:16:12 UTC
Permalink
Post by Martin Simmons
Post by Konstantin Belousov
One process which executed pthread_barrier_init(), performed what you
proposed. What should do the pthread_barrier_wait() call in another
process, which shares the 'barrier' with the first process, but does
not share the whole address space ? After your pthread_barrier_init()
executed, barrier contains the address of the object (off-page) in the
other address space, for that process.
Ah, sorry, I understand now (the init functions are called before any
sharing).
Well, the memory is either already shared between processes, or it should
become shared before other process may operate on the object carried by
the memory. It is that pthread_mutex_init() must be called before any
other pthread_mutex_*() functions, but only once globally.
Post by Martin Simmons
How should the destroy functions be used by the processes? I.e. should only
the "last" process call destroy or can every process call it?
Hm, this is a good observation. pthread_mutex_destroy() must be called
only by last process, i.e. no other pthread_mutex_* calls are allowed for
the object after the _destroy() was called.

What this means for my implementation, is that processes other than the
_destroy() caller keep the record in the pshared cache, and this needs
fixing. For now, I added a mechanism which scans the whole hash and
re-checks the validity on any object destroy. This can be optimized,
e.g. by doing the scan only each N calls to _destroy(), or by scanning
only the same hash chain, or by not doing this at all. I think it is
an acceptable compromise for now.

A specific test for the case is at
https://www.kib.kiev.ua/kib/pshared/pthread_shared_destroy.c

Updated patch
https://www.kib.kiev.ua/kib/pshared/pshared.4.patch
Eric van Gyzen
2016-02-15 21:39:18 UTC
Permalink
Post by Eric van Gyzen
Post by Konstantin Belousov
The implementation in the patch
https://www.kib.kiev.ua/kib/pshared/pshared.1.patch
gives shared mutexes, condvars, rwlocks and barriers.
I reviewed everything except kern_umtx.c, which I plan to review on
Monday.
My only comment on kern_umtx.c is, why are the permission checks compiled out?

I also reviewed versions 2 and 3; the revisions look fine.

Thanks for the chance to review, and for waiting for me to find time.

Eric
Konstantin Belousov
2016-02-16 11:32:22 UTC
Permalink
Post by Eric van Gyzen
My only comment on kern_umtx.c is, why are the permission checks compiled out?
Assume that we changed the ABI of libthr and shared locks do not require
an offpage. Then, access to the locks is completely controlled by the
access to the page containing the lock. If a process can mmap the page,
it can own the lock.

From this point of view, access to the offpage shared memory object
is the same as the access to the key page. Which is the reason to not
include the access permissions checks.

On the other hand, if you have something in kernel, which also owns a
reference on ucred (for other means), you sure consider whether the usual
access control is appropriate. Mostly, I leave the #ifdef-ed checks
to reconsider it later, which worked since you asked the question. I
definitely do not see much use of the shm_access() checks, but I am not
completely sure about possible mac policies utilization there, although
I do not really think they could be usefully attached to the app-level
locks.
Post by Eric van Gyzen
I also reviewed versions 2 and 3; the revisions look fine.
Thank you very much.
Eric van Gyzen
2016-02-16 14:56:32 UTC
Permalink
Post by Konstantin Belousov
Post by Eric van Gyzen
My only comment on kern_umtx.c is, why are the permission checks compiled out?
Assume that we changed the ABI of libthr and shared locks do not require
an offpage. Then, access to the locks is completely controlled by the
access to the page containing the lock. If a process can mmap the page,
it can own the lock.
From this point of view, access to the offpage shared memory object
is the same as the access to the key page. Which is the reason to not
include the access permissions checks.
This makes sense.
Post by Konstantin Belousov
On the other hand, if you have something in kernel, which also owns a
reference on ucred (for other means), you sure consider whether the usual
access control is appropriate.
This sounds wise. I'll keep it in mind.
Post by Konstantin Belousov
I
definitely do not see much use of the shm_access() checks, but I am not
completely sure about possible mac policies utilization there, although
I do not really think they could be usefully attached to the app-level
locks.
I would tend to agree, but I haven't used MAC for several years, so I'm
not sure either.

Cheers,

Eric
Daniel Eischen
2016-02-16 17:27:12 UTC
Permalink
Post by Konstantin Belousov
Post by Eric van Gyzen
My only comment on kern_umtx.c is, why are the permission checks compiled out?
Assume that we changed the ABI of libthr and shared locks do not require
an offpage.
How are you coming with that? I thought maybe you were going to think
about making the synch types structs, but not actually changing the
implementation (yet).
--
DE
Konstantin Belousov
2016-02-17 16:45:41 UTC
Permalink
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Eric van Gyzen
My only comment on kern_umtx.c is, why are the permission checks compiled out?
Assume that we changed the ABI of libthr and shared locks do not require
an offpage.
How are you coming with that? I thought maybe you were going to think
about making the synch types structs, but not actually changing the
implementation (yet).
Are you asking what is the state of the work for changing the libthr
ABI by replacing object pointers with inline structures, am I reading
the question right ? (Sorry, english is not my native language)

I do plan to introduce inlined objects (most likely in the form of
libthr2 initially, i.e. cc -D_LIBTHR2 -o file file.c -lthr2). But my
plans are to get the existing patch for pshared into the tree for 11.0.
After that I wanted to implement robust mutexes, still in the context of
the libthr. Then libthr2.

Finishing the current patch for 11.0 is the immediate goal, while I did
not forget neither abandoned the inline alternative and do plan to work
on it.
Daniel Eischen
2016-02-17 17:37:52 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
Post by Konstantin Belousov
Post by Eric van Gyzen
My only comment on kern_umtx.c is, why are the permission checks compiled out?
Assume that we changed the ABI of libthr and shared locks do not require
an offpage.
How are you coming with that? I thought maybe you were going to think
about making the synch types structs, but not actually changing the
implementation (yet).
Are you asking what is the state of the work for changing the libthr
ABI by replacing object pointers with inline structures, am I reading
the question right ? (Sorry, english is not my native language)
I am sorry, my question was not phrased very well. Sometimes I
am amazed that non-native English speakers can write better English
than native English speakers :-) But, yes, that was my question.
Post by Konstantin Belousov
I do plan to introduce inlined objects (most likely in the form of
libthr2 initially, i.e. cc -D_LIBTHR2 -o file file.c -lthr2). But my
plans are to get the existing patch for pshared into the tree for 11.0.
After that I wanted to implement robust mutexes, still in the context of
the libthr. Then libthr2.
As soon as this is done, will we build FreeBSD base OS against
the new API? So only ports would be affected.

I was hoping to see partially inlined objects (with the first word
being a pointer to the allocated object, just like now) in for 11.0.
So by 12.0 we could make the switch over to fully inlined.

If we introduce either partially or fully inlined objects for 11.1
(with your libthr2), how does that affect our ability to move to
fully inlined by default (base & ports) for 12.0? We would not
have had a full release cycle for the ABI change.
Post by Konstantin Belousov
Finishing the current patch for 11.0 is the immediate goal, while I did
not forget neither abandoned the inline alternative and do plan to work
on it.
Thanks!
--
DE
Daniel Eischen
2016-02-18 17:09:59 UTC
Permalink
Post by Daniel Eischen
Post by Konstantin Belousov
I do plan to introduce inlined objects (most likely in the form of
libthr2 initially, i.e. cc -D_LIBTHR2 -o file file.c -lthr2). But my
plans are to get the existing patch for pshared into the tree for 11.0.
After that I wanted to implement robust mutexes, still in the context of
the libthr. Then libthr2.
As soon as this is done, will we build FreeBSD base OS against
the new API? So only ports would be affected.
I do not think this is feasible. Base system does not have any significant
thread consumers, might be only ntpd qualifies. Small things like ngctl
are not that important.
But base system provides C++ runtime for ports and I suspect that libc++
depends on the libthr ABI. Even jemalloc depends on libthr ABI. So
changing only the base ABI is probably impossible, from the first look
the switch like WITH_LIBTHR2_DEFAULT would be a flag day. Anyway, this
must be considered carefully during the later stage of the libthr2
development, right now it is rather empty speculation on my side.
I would think partially inlined objects (still a pointer inside)
could be made in libthr (not libthr2) by default for 11.0 (or 11.x).
So that the only change is the size of the objects, not anything
else. The only breakage would be layout related.
--
DE
Konstantin Belousov
2016-02-19 00:32:55 UTC
Permalink
Post by Daniel Eischen
But base system provides C++ runtime for ports and I suspect that libc++
depends on the libthr ABI. Even jemalloc depends on libthr ABI. So
changing only the base ABI is probably impossible, from the first look
the switch like WITH_LIBTHR2_DEFAULT would be a flag day. Anyway, this
must be considered carefully during the later stage of the libthr2
development, right now it is rather empty speculation on my side.
This paragraph is relevant for my answer below.
Post by Daniel Eischen
I would think partially inlined objects (still a pointer inside)
could be made in libthr (not libthr2) by default for 11.0 (or 11.x).
So that the only change is the size of the objects, not anything
else. The only breakage would be layout related.
Structure size is part of the library ABI, when the structure is exposed
to user, it cannot be increased without consequences.

Changing the size of the libthr objects changes layout of the objects
embedding the locks. Doing
class MyLock {
private:
pthread_mutex_t m_lock;
const char *name;
};
is the common and very popular practice. With the change proposed to
libthr.so.3, you get subtle and sudden ABI breakage for all libthr
consumers as well. In particular, it would affect libc (jemalloc) and
libc++. Depending on the variant of headers used for the compilation,
you get different layout for user structures.
Konstantin Belousov
2016-02-19 15:08:27 UTC
Permalink
Post by Konstantin Belousov
Post by Daniel Eischen
But base system provides C++ runtime for ports and I suspect that libc++
depends on the libthr ABI. Even jemalloc depends on libthr ABI. So
changing only the base ABI is probably impossible, from the first look
the switch like WITH_LIBTHR2_DEFAULT would be a flag day. Anyway, this
must be considered carefully during the later stage of the libthr2
development, right now it is rather empty speculation on my side.
This paragraph is relevant for my answer below.
Post by Daniel Eischen
I would think partially inlined objects (still a pointer inside)
could be made in libthr (not libthr2) by default for 11.0 (or 11.x).
So that the only change is the size of the objects, not anything
else. The only breakage would be layout related.
Structure size is part of the library ABI, when the structure is exposed
to user, it cannot be increased without consequences.
Changing the size of the libthr objects changes layout of the objects
embedding the locks. Doing
class MyLock {
pthread_mutex_t m_lock;
const char *name;
};
is the common and very popular practice. With the change proposed to
libthr.so.3, you get subtle and sudden ABI breakage for all libthr
consumers as well. In particular, it would affect libc (jemalloc) and
libc++. Depending on the variant of headers used for the compilation,
you get different layout for user structures.
libc is part of FreeBSD, so it would be recompiled for the new
size. I was also assuming library version bumps.
Increasing the structures sizes (and bumping versions) would
- break ABI
- invalidate all the work which was done by people from the FreeBSD 7.x
time to keep the ABI stable, by writing shims, by adopting changes
to provide compatibility, and by testing the compatibility
without any benefit of a new feature. It would be only promised to
provide a new feature sometime in the 11.x scope.

My immediate goal is to get the published patch committed.
After that, I want to look at the implementation of the robust
mutexes. I hope that this would be done for 11.0, but if not, it
should be mergeable.

The libthr2 stuff, by which I call everything related to inlining,
should be started after that.

Loading...