Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

pthread library related

4 views
Skip to first unread message

Charles Cui

unread,
May 18, 2016, 6:47:03 PM5/18/16
to
Hi guys,


I spent some time on pthread libraries in netbsd which is a big part in
my project, and have some questions to ask.
Taking pthread_barrierattr_getpshared, pthread_barrierattr_setpshared
as an example. These functions are used to getting and setting barrierattr
data structure. netbsd has several fields in the data structure, while
freebsd has the only pshared field. I am wondering which fields in the
implementation of netbsd can be used as pshare in netbsd?
Following are definitions of netbsd vs freebsd, the same problems exist in
mutexattr, condattr and rwlockattr. Also are there any documents which
describe implementation details in netbsd? It would be great if we have.

*functions with missing features*:
pthread_barrierattr_getpshared
pthread_barrierattr_setpshared
*netbsd *
struct __pthread_barrier_st {
unsigned int ptb_magic;

/* Protects data below */
pthread_spin_t ptb_lock;

pthread_queue_t ptb_waiters;
unsigned int ptb_initcount;
unsigned int ptb_curcount;
unsigned int ptb_generation;

void *ptb_private;
};

*freebsd*
struct pthread_barrierattr {
int pshared;
};

*functions with missing features:*
pthread_mutexattr_setpshared
pthread_mutexattr_getpshared
pthread_mutexattr_getprioceiling
pthread_mutexattr_setprioceiling
pthread_mutexattr_getprotocol
pthread_mutexattr_setprotocol

*netbsd*:
struct __pthread_mutexattr_st {
unsigned int ptma_magic;
void *ptma_private;
};

*freebsd*:
struct pthread_mutex_attr {
enum pthread_mutextype m_type;
int m_protocol;
int m_ceiling;
int m_pshared;
};

*functions with missing features*
pthread_condattr_setpshared
pthread_condattr_getpshared
pthread_condattr_getclock

*netbsd*
struct __pthread_condattr_st {
unsigned int ptca_magic;
void *ptca_private;
};
*freebsd*
struct pthread_cond_attr {
int c_pshared;
int c_clockid;
};

* functions with missing features*
pthread_rwlockattr_getpshared
pthread_rwlockattr_setpshared

*netbsd*
struct __pthread_rwlockattr_st {
unsigned int ptra_magic;
void *ptra_private;
};
*freebsd*
struct pthread_rwlockattr {
int pshared;
};

Martin Husemann

unread,
May 18, 2016, 7:13:18 PM5/18/16
to
On Wed, May 18, 2016 at 03:40:38PM -0700, Charles Cui wrote:
> *functions with missing features*:
> pthread_barrierattr_getpshared
> pthread_barrierattr_setpshared

Effectively:

int pthread_barrierattr_getpshared(const pthread_barrierattr_t
*restrict attr, int *restrict pshared)
{
*pshared = PTHREAD_PROCESS_PRIVATE;
}

int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
int pshared)
{
return EINVAL;
}


Not making this functionality optional any more is a mistake in Posix IMHO.

Martin

SODA Noriyuki

unread,
May 18, 2016, 10:00:57 PM5/18/16
to
>>>>> On Thu, 19 May 2016 01:13:09 +0200,
Martin Husemann <mar...@duskware.de> said:

> int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
> int pshared)
> {
> return EINVAL;
> }

I think the following is better:

int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
int pshared)
{
switch (pshared) {
case PTHREAD_PROCESS_PRIVATE:
return 0;
case PTHREAD_PROCESS_SHARED:
return ENOSYS;
}
return EINVAL;
}
--
soda

SODA Noriyuki

unread,
May 18, 2016, 11:45:09 PM5/18/16
to
>>>>> On Wed, 18 May 2016 20:31:08 -0700,
Charles Cui <charles...@gmail.com> said:

> How about other functions? Does it make sense to follow POSIX standard
> to implement the missing functions in netbsd?

In general, it's not recommended to add a stub function which returns
ENOSYS (i.e. "Function not implemented") unless there is strong reason
to do so.
Because autoconf fails to detect that the function is not really
implemented, and fails to provide alternate implementation that
the autoconf'ed application includes.
And because there are many applications which do not check the
ENOSYS erorr correctly and fails to work.
--
soda

SODA Noriyuki

unread,
May 18, 2016, 11:49:14 PM5/18/16
to
Forgot to say one thing...

>>>>> On Thu, 19 May 2016 12:44:57 +0900, SODA Noriyuki <so...@NetBSD.org> said:

>> How about other functions? Does it make sense to follow POSIX standard
>> to implement the missing functions in netbsd?

> In general, it's not recommended to add a stub function which returns
> ENOSYS (i.e. "Function not implemented") unless there is strong reason
> to do so.

Adding real implementation (instead of the ENOSYS stub) is desired,
of course...
--
soda

Martin Husemann

unread,
May 19, 2016, 4:37:10 AM5/19/16
to
On Thu, May 19, 2016 at 12:49:07PM +0900, SODA Noriyuki wrote:
> Adding real implementation (instead of the ENOSYS stub) is desired,
> of course...

In this case: is it even possible in a portable way for all of our
architectures?

Martin

SODA Noriyuki

unread,
May 19, 2016, 5:06:27 AM5/19/16
to
>>>>> On Thu, 19 May 2016 10:36:57 +0200,
Martin Husemann <mar...@duskware.de> said:

>> Adding real implementation (instead of the ENOSYS stub) is desired,
>> of course...

> In this case: is it even possible in a portable way for all of our
> architectures?

Yes.
Easiest way is to call a locking system call always, in the case of
PTHREAD_PROCESS_SHARED.
--
soda

Martin Husemann

unread,
May 19, 2016, 5:22:08 AM5/19/16
to
On Thu, May 19, 2016 at 06:06:19PM +0900, SODA Noriyuki wrote:
> Yes.
> Easiest way is to call a locking system call always, in the case of
> PTHREAD_PROCESS_SHARED.

Oh, didn't think of that - nice, and only hits the stupid shared case
(performance wise).

Martin

Christos Zoulas

unread,
May 19, 2016, 10:12:08 AM5/19/16
to
On May 19, 11:00am, so...@yuruyuru.net (SODA Noriyuki) wrote:
-- Subject: Re: pthread library related

| >>>>> On Thu, 19 May 2016 01:13:09 +0200,
| Martin Husemann <mar...@duskware.de> said:
|
| > int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
| > int pshared)
| > {
| > return EINVAL;
| > }
|
| I think the following is better:
|
| int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
| int pshared)
| {
| switch (pshared) {
| case PTHREAD_PROCESS_PRIVATE:
| return 0;
| case PTHREAD_PROCESS_SHARED:
| return ENOSYS;
| }
| return EINVAL;
| }

Yes, that's better.

christos

Christos Zoulas

unread,
May 19, 2016, 1:36:43 PM5/19/16
to
On May 18, 3:40pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: pthread library related

| Hi guys,
|
|
| I spent some time on pthread libraries in netbsd which is a big part in
| my project, and have some questions to ask.
| Taking pthread_barrierattr_getpshared, pthread_barrierattr_setpshared
| as an example. These functions are used to getting and setting barrierattr
| data structure. netbsd has several fields in the data structure, while
| freebsd has the only pshared field. I am wondering which fields in the
| implementation of netbsd can be used as pshare in netbsd?
| Following are definitions of netbsd vs freebsd, the same problems exist in
| mutexattr, condattr and rwlockattr. Also are there any documents which
| describe implementation details in netbsd? It would be great if we have.

You can see how FreeBSD is implementing them; it is a lot of code to do
this and would require some architectural review. The relevant files are:

http://nxr.netbsd.org/xref/src-freebsd/lib/libthr/thread/thr_pshared.c
http://nxr.netbsd.org/xref/src-freebsd/lib/libthr/thread/thr_barrierattr.c
http://nxr.netbsd.org/xref/src-freebsd/sys/kern/kern_umtx.c

We don't have such mutex functionality in our kernel. Implementing this
would be a GSoC project in itself.

christos

Charles Cui

unread,
May 19, 2016, 8:38:04 PM5/19/16
to
Thanks Christos, Martin and SODA for these advices and documents!
I will start to get familiar with freebsd designs and try to contribute as
much
as possible to netbsd community.

Charles Cui

unread,
May 19, 2016, 8:38:07 PM5/19/16
to
Thanks Martin and Soda!
How about other functions? Does it make sense to follow POSIX standard
to implement the missing functions in netbsd?

2016-05-18 19:00 GMT-07:00 SODA Noriyuki <so...@yuruyuru.net>:

> >>>>> On Thu, 19 May 2016 01:13:09 +0200,
> Martin Husemann <mar...@duskware.de> said:
>
> > int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
> > int pshared)
> > {
> > return EINVAL;
> > }
>
> I think the following is better:
>
> int pthread_barrierattr_setpshared(pthread_barrierattr_t *attr,
> int pshared)
> {
> switch (pshared) {
> case PTHREAD_PROCESS_PRIVATE:
> return 0;
> case PTHREAD_PROCESS_SHARED:
> return ENOSYS;
> }
> return EINVAL;
> }
> --
> soda
>

Joerg Sonnenberger

unread,
May 20, 2016, 5:15:58 AM5/20/16
to
On Thu, May 19, 2016 at 01:36:29PM -0400, Christos Zoulas wrote:
> You can see how FreeBSD is implementing them; it is a lot of code to do
> this and would require some architectural review. The relevant files are:
>
> http://nxr.netbsd.org/xref/src-freebsd/lib/libthr/thread/thr_pshared.c
> http://nxr.netbsd.org/xref/src-freebsd/lib/libthr/thread/thr_barrierattr.c
> http://nxr.netbsd.org/xref/src-freebsd/sys/kern/kern_umtx.c
>
> We don't have such mutex functionality in our kernel. Implementing this
> would be a GSoC project in itself.

I don't think we want to use futexes in general. I'm not even sure I
care about performance for something horrible like "robust" mutexes at
all. A good starting point might to just extend the existing semaphores,
if necessary.

Joerg

Kamil Rytarowski

unread,
May 20, 2016, 5:42:25 AM5/20/16
to
Robust POSIX mutexes are now needed in the .NET platform to implement
named mutexes:

"Add named mutex for cross-process synchronization "

https://github.com/dotnet/coreclr/commit/249221697fc5cf18c07566bac0e9f0eb6525218a

I've opened an upstream issue to track this in NetBSD: " Robust POSIX
mutexes unavailable on NetBSD #5128 "
https://github.com/dotnet/coreclr/issues/5128

A short example of what is needed is presented here (CMake test
extracted from the mentioned patch):

#include <errno.h>
#include <pthread.h>
#include <time.h>

int main()
{
pthread_mutexattr_t mutexAttributes;
pthread_mutexattr_init(&mutexAttributes);
pthread_mutexattr_setpshared(&mutexAttributes, PTHREAD_PROCESS_SHARED);
pthread_mutexattr_settype(&mutexAttributes, PTHREAD_MUTEX_RECURSIVE);
pthread_mutexattr_setrobust(&mutexAttributes, PTHREAD_MUTEX_ROBUST);

pthread_mutex_t mutex;
pthread_mutex_init(&mutex, &mutexAttributes);

pthread_mutexattr_destroy(&mutexAttributes);+
struct timespec timeoutTime;
timeoutTime.tv_sec = 1; // not the right way to specify absolute
time, but just checking availability of timed lock
timeoutTime.tv_nsec = 0;
pthread_mutex_timedlock(&mutex, &timeoutTime);
pthread_mutex_consistent(&mutex);

pthread_mutex_destroy(&mutex);

int error = EOWNERDEAD;
error = ENOTRECOVERABLE;
error = ETIMEDOUT;
error = 0;
return error;
}

Christos Zoulas

unread,
May 26, 2016, 5:35:50 PM5/26/16
to
On May 26, 2:08pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

| Hi Christos,
|
| I am studying the pthread libraries in netbsd, and have several patches
| completed.
| I have attached all of them, which is pretty easy, but can fix some
| problems in user land.
| Note that some of them needs to add more logic. If you can, please give me
| some comments on them.

I'll send a separate mail.

| For the pthread part, I found a deep understanding is necessary to
| implement the feature
| such as cross process synchronization. Also, I found a patch which is
| written by ad (http://www.netbsd.org/~ad/prio_protect.diff)
| I found that patch implements some functions that I need, I am right now
| focusing on how to port
| that patch to my work.

Sounds good. I would also look at the FreeBSD implementation.

| Also, there are some missing macros like _SC_<MISS_MICRO>, I found the
| existing code just return the macro
| using _POSIX_<MISS_MICRO>, I am wondering where do you guys enforce the
| limitation in the kernel ?
| To clarify, let's use _SC_TIMER_MAX as an example, the netbsd code just
| returns _POSIX_TIMER_MAX in
| sysconf system call, where can I find the logic to enforce this number when
| user land allocates more times than this value?

This might sched some light.

http://nxr.netbsd.org/search?q=&project=src&defs=&refs=TIMER_MAX&path=&hist=

| Let me know if there are any concerns.

No, I will send a separate mail with more comments about the source. Please
note that with source commits you should also supply unit-tests (see
http://nxr.netbsd.org/xref/src/tests/lib/libpthread/) for example and also
manual pages. I can help you with both.

christos

Christos Zoulas

unread,
May 26, 2016, 6:02:42 PM5/26/16
to
On May 26, 2:08pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

1. SIGRTMIN/SIGRTMAX should not be exposed (the kernel does not support
the functionality).
2. I guess the bsd_signal is ok, but I don't like it :-)
http://pubs.opengroup.org/onlinepubs/009695399/functions/bsd_signal.html
3. The pthread_condattr_getclock() prototype is wrong:
http://pubs.opengroup.org/onlinepubs/009695399/functions/pthread_condattr_getclock.html
4. pthread_barrierattr_{get,set}pshared. I am ambivalent about that one because
I am afraid programs might discover it and think erroneously that we support
it. Let's wait on that.
5. _SC_SEM_NSEMS_MAX, although that's not the real limit, good enough.

So let's not do 1, lets do 2, 5, fix 3 and wait for 4 until we understand
it better.

christos

Christos Zoulas

unread,
May 26, 2016, 6:09:54 PM5/26/16
to
On May 26, 3:05pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

| This is really helpful, actually, I searched _SC_TIMER_MAX and
| _POSIX_TIMER_MAX in nxr,
| but the results cannot indicate where these logics can be found.
| One more question, does other _SC_<MISS_MACRO> has the same naming
| convention
| in the kernel? I mean can I always search MISS_MICRO to find the logic for
| this variable?

I wish, but it is something to try. What I do instead look in the kernel
for the function that needs to enforce the limit. for example, sem_open
src/lib/librt/sem.c -> _ksem_open -> src/sys/kern/uipc_sem.c...

christos

Charles Cui

unread,
May 26, 2016, 7:07:17 PM5/26/16
to
Yes, I can do 2 and 5, and fix 3.
I am now in the progress of porting ad's patch, let's complete it.
Once I have verified the porting part is done (compile new kernel , run
benchmarks, see errors does not exist),
I will return to complete 2, 5 and fix 3, sounds good?
Of course, I will send you guys the patch of the porting work shortly.


2016-05-26 15:02 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On May 26, 2:08pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: pthread library related
>

Christos Zoulas

unread,
May 26, 2016, 7:17:15 PM5/26/16
to
On May 26, 3:27pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

| Yes, I can do 2 and 5, and fix 3.
| I am now in the progress of porting ad's patch, let's complete it.
| Once I have verified the porting part is done (compile new kernel , run
| benchmarks, see errors does not exist),
| I will return to complete 2, 5 and fix 3, sounds good?
| Of course, I will send you guys the patch of the porting work shortly.

Sounds good, don't forget man pages and tests.

christos

Kamil Rytarowski

unread,
May 26, 2016, 7:27:21 PM5/26/16
to


On 27.05.2016 00:02, Christos Zoulas wrote:
> 5. _SC_SEM_NSEMS_MAX, although that's not the real limit, good enough.

Is it the same (indirectly) SEM_VALUE_MAX?

#define SEM_VALUE_MAX (~0U)

It's located in <sys/semaphore.h>.

I think we should go for LONG_MAX:
1. We will be able to return it via sysconf(3)
2. It's still large enough
3. Change SEM_VALUE_MAX to LONG_MAX to retain the same corresponding size.

Also we perhaps should add a dynamic value of SEM_VALUE_MAX for
sysconf(3) and maybe another value for the longest valid path name size
(lately it was around 15 characters).

Charles Cui

unread,
May 27, 2016, 12:19:25 PM5/27/16
to
Please see my comments inline.

2016-05-26 14:35 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On May 26, 2:08pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: pthread library related
>
This is really helpful, actually, I searched _SC_TIMER_MAX and
_POSIX_TIMER_MAX in nxr,
but the results cannot indicate where these logics can be found.
One more question, does other _SC_<MISS_MACRO> has the same naming
convention
in the kernel? I mean can I always search MISS_MICRO to find the logic for
this variable?

>
>
> | Let me know if there are any concerns.
>
> No, I will send a separate mail with more comments about the source. Please
> note that with source commits you should also supply unit-tests (see
> http://nxr.netbsd.org/xref/src/tests/lib/libpthread/) for example and also
> manual pages. I can help you with both.
>
Yes, I agree, I will try to add those, let me know if there are examples or
rules to follow.

>
> christos
>

Martin Husemann

unread,
May 30, 2016, 2:31:10 PM5/30/16
to
On Mon, May 30, 2016 at 10:33:33AM -0700, Charles Cui wrote:

> The second one is the reimplementation of bsd signal.

Do we really need this? Why is it a function pointer?

> The third one is the missing kernel logic for _SC_SEM_NSEMS_MAX.

Why not use atomic_inc/atomic_dec for the p_nsem count (and a proper
atomic-friendly type instead of uint)? Why limit the number at all?

Martin

Christos Zoulas

unread,
May 30, 2016, 3:28:19 PM5/30/16
to
On May 30, 8:29pm, mar...@duskware.de (Martin Husemann) wrote:
-- Subject: Re: pthread library related

| Do we really need this? Why is it a function pointer?

It is the same as signal.

| > The third one is the missing kernel logic for _SC_SEM_NSEMS_MAX.
|
| Why not use atomic_inc/atomic_dec for the p_nsem count (and a proper
| atomic-friendly type instead of uint)? Why limit the number at all?

Sure.

christos

Martin Husemann

unread,
May 30, 2016, 3:32:54 PM5/30/16
to
On Mon, May 30, 2016 at 03:28:09PM -0400, Christos Zoulas wrote:
> On May 30, 8:29pm, mar...@duskware.de (Martin Husemann) wrote:
> -- Subject: Re: pthread library related
>
> | Do we really need this? Why is it a function pointer?
>
> It is the same as signal.

OK. Why is "signal" a function pointer?

Martin

Christos Zoulas

unread,
May 30, 2016, 3:34:49 PM5/30/16
to
On May 30, 9:32pm, mar...@duskware.de (Martin Husemann) wrote:
-- Subject: Re: pthread library related

| On Mon, May 30, 2016 at 03:28:09PM -0400, Christos Zoulas wrote:
| > On May 30, 8:29pm, mar...@duskware.de (Martin Husemann) wrote:
| > -- Subject: Re: pthread library related
| >
| > | Do we really need this? Why is it a function pointer?
| >
| > It is the same as signal.
|
| OK. Why is "signal" a function pointer?

It is not a function pointer; it is a function that takes as an argument
the new signal handler (function pointer) returns a function
pointer (the old signal handler function)

christos

Christos Zoulas

unread,
May 30, 2016, 3:55:02 PM5/30/16
to
On May 30, 10:33am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

Looks good, but be careful with whitespace in general:

@@ -316,7 +316,7 @@ ksem_create(lwp_t *l, const char *name, ksem_t **ksret, mode_t mode, u_int val)
if (val > SEM_VALUE_MAX) {
return EINVAL;
}
-
+

Why extra whitespace here.

if (name != NULL) {
len = strlen(name);
if (len > SEM_MAX_NAMELEN) {
@@ -333,6 +333,14 @@ ksem_create(lwp_t *l, const char *name, ksem_t **ksret, mode_t mode, u_int val)
len = 0;
}

+ mutex_enter(&ksem_lock);
+ if (l->l_proc->p_nsems >= SEM_NSEMS_MAX) {
+ mutex_exit(&ksem_lock);
+ return -1;
+ }
+ l->l_proc->p_nsems += 1;
+ mutex_exit(&ksem_lock);
+

Mixed tabs and spaces. Better use atomics.

ks = kmem_zalloc(sizeof(ksem_t), KM_SLEEP);
mutex_init(&ks->ks_lock, MUTEX_DEFAULT, IPL_NONE);
cv_init(&ks->ks_cv, "psem");
@@ -347,6 +355,7 @@ ksem_create(lwp_t *l, const char *name, ksem_t **ksret, mode_t mode, u_int val)
ks->ks_gid = kauth_cred_getegid(uc);

atomic_inc_uint(&nsems_total);
+
*ksret = ks;
return 0;
}

Avoid touching code that has nothing to do with your change.

@@ -366,6 +375,10 @@ ksem_free(ksem_t *ks)
kmem_free(ks, sizeof(ksem_t));

atomic_dec_uint(&nsems_total);
+
+ mutex_enter(&ksem_lock);
+ curproc->p_nsems -= 1;
+ mutex_exit(&ksem_lock);

Better use atomics

}

int

Christos Zoulas

unread,
May 30, 2016, 4:02:39 PM5/30/16
to
On May 30, 10:33am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

Inline comments.

christos

diff --git a/lib/libpthread/pthread.h b/lib/libpthread/pthread.h
index 5970378..ed16d7c 100644
--- a/lib/libpthread/pthread.h
+++ b/lib/libpthread/pthread.h
@@ -94,6 +94,9 @@ int pthread_mutex_destroy(pthread_mutex_t *);
int pthread_mutex_lock(pthread_mutex_t *);
int pthread_mutex_trylock(pthread_mutex_t *);
int pthread_mutex_unlock(pthread_mutex_t *);
+int pthread_mutex_timedlock(pthread_mutex_t *, const struct timespec*__restrict);
+int pthread_mutex_getprioceiling(const pthread_mutex_t *__restrict, int*__restrict);
+int pthread_mutex_setprioceiling(pthread_mutex_t* __restrict, int, int*__restrict);

Wrap lines, whitespace "const struct timespec *__restrict" etc. (space after
the type name)

+int pthread_mutexattr_getprotocol(const pthread_mutexattr_t * __restrict, int*__restrict);
+int pthread_mutexattr_setprotocol(pthread_mutexattr_t* , int);
+int pthread_mutexattr_getprioceiling(const pthread_mutexattr_t *__restrict,int*__restrict);
+int pthread_mutexattr_setprioceiling(pthread_mutexattr_t*, int);

Again

int pthread_cond_init(pthread_cond_t * __restrict,
const pthread_condattr_t * __restrict);
int pthread_cond_destroy(pthread_cond_t *);
@@ -116,7 +122,7 @@ int pthread_cond_broadcast(pthread_cond_t *);
int pthread_condattr_init(pthread_condattr_t *);
#if defined(_NETBSD_SOURCE)
int pthread_condattr_setclock(pthread_condattr_t *, clockid_t);
-int pthread_condattr_getclock(pthread_condattr_t *);
+int pthread_condattr_getclock(const pthread_condattr_t *__restrict attr, clockid_t *__restrict clock_id);

Again (wrap)

int
-pthread_condattr_getclock(pthread_condattr_t *attr)
+pthread_condattr_getclock(const pthread_condattr_t *__restrict attr, clockid_t *__restrict clock_id)

Wrap

{
if (attr == NULL || attr->ptca_private == NULL)
return EINVAL;
- return *(int *)attr->ptca_private;
+ *clock_id = *(clockid_t*)attr->ptca_private;

Whitespace before *

+ return 0;
}

int
diff --git a/lib/libpthread/pthread_mutex.c b/lib/libpthread/pthread_mutex.c
index e755ade..ec46070 100644
--- a/lib/libpthread/pthread_mutex.c
+++ b/lib/libpthread/pthread_mutex.c
@@ -51,6 +51,7 @@ __RCSID("$NetBSD: pthread_mutex.c,v 1.59 2014/02/03 15:51:01 rmind Exp $");

#include <sys/types.h>
#include <sys/lwpctl.h>
+#include <sys/sched.h>
#include <sys/lock.h>

#include <errno.h>
@@ -67,10 +68,12 @@ __RCSID("$NetBSD: pthread_mutex.c,v 1.59 2014/02/03 15:51:01 rmind Exp $");
#define MUTEX_WAITERS_BIT ((uintptr_t)0x01)
#define MUTEX_RECURSIVE_BIT ((uintptr_t)0x02)
#define MUTEX_DEFERRED_BIT ((uintptr_t)0x04)
+#define MUTEX_PROTECT_BIT ((uintptr_t)0x08)
#define MUTEX_THREAD ((uintptr_t)-16L)

#define MUTEX_HAS_WAITERS(x) ((uintptr_t)(x) & MUTEX_WAITERS_BIT)
#define MUTEX_RECURSIVE(x) ((uintptr_t)(x) & MUTEX_RECURSIVE_BIT)
+#define MUTEX_PROTECT(x) ((uintptr_t)(x) & MUTEX_PROTECT_BIT)
#define MUTEX_OWNER(x) ((uintptr_t)(x) & MUTEX_THREAD)

#if __GNUC_PREREQ__(3, 0)
@@ -80,7 +83,7 @@ __RCSID("$NetBSD: pthread_mutex.c,v 1.59 2014/02/03 15:51:01 rmind Exp $");
#endif

static void pthread__mutex_wakeup(pthread_t, pthread_mutex_t *);
-static int pthread__mutex_lock_slow(pthread_mutex_t *);
+static int pthread__mutex_lock_slow(pthread_mutex_t *, const struct timespec*);

wrap and space

static int pthread__mutex_unlock_slow(pthread_mutex_t *);
static void pthread__mutex_pause(void);

@@ -103,16 +106,21 @@ __strong_alias(__libc_mutexattr_settype,pthread_mutexattr_settype)
int
pthread_mutex_init(pthread_mutex_t *ptm, const pthread_mutexattr_t *attr)
{
- intptr_t type;
+ uintptr_t type, proto, val, ceil;

if (__predict_false(__uselibcstub))
return __libc_mutex_init_stub(ptm, attr);

- if (attr == NULL)
+ if (attr == NULL) {
type = PTHREAD_MUTEX_NORMAL;
- else
- type = (intptr_t)attr->ptma_private;
-
+ proto = PTHREAD_PRIO_NONE;
+ ceil = 0;
+ } else {
+ val = (uintptr_t)attr->ptma_private;
+ type = val & 0xff;
+ proto = (val & 0xff00) >> 8;
+ ceil = (val & 0xff0000) >> 16;
+ }
switch (type) {
case PTHREAD_MUTEX_ERRORCHECK:
__cpu_simple_lock_set(&ptm->ptm_errorcheck);
@@ -127,10 +135,17 @@ pthread_mutex_init(pthread_mutex_t *ptm, const pthread_mutexattr_t *attr)
ptm->ptm_owner = NULL;
break;
}
-
+ switch (proto) {
+ case PTHREAD_PRIO_PROTECT:
+ val = (uintptr_t)ptm->ptm_owner;
+ val |= MUTEX_PROTECT_BIT;
+ ptm->ptm_owner = (void*)val;

space

+ break;
+ }
ptm->ptm_magic = _PT_MUTEX_MAGIC;
ptm->ptm_waiters = NULL;
ptm->ptm_recursed = 0;
+ ptm->ptm_ceiling = (unsigned char)ceil;

return 0;
}
@@ -168,7 +183,24 @@ pthread_mutex_lock(pthread_mutex_t *ptm)
#endif
return 0;
}
- return pthread__mutex_lock_slow(ptm);
+ return pthread__mutex_lock_slow(ptm, NULL);
+}
+
+int
+pthread_mutex_timedlock(pthread_mutex_t* ptm, const struct timespec *ts)
+{
+ pthread_t self;
+ void *val;
+
+ self = pthread__self();
+ val = atomic_cas_ptr(&ptm->ptm_owner, NULL, self);
+ if (__predict_true(val == NULL)) {
+#ifndef PTHREAD__ATOMIC_IS_MEMBAR
+ membar_enter();
+#endif
+ return 0;
+ }
+ return pthread__mutex_lock_slow(ptm, ts);
}

/* We want function call overhead. */
@@ -258,11 +290,12 @@ again:
}

NOINLINE static int
-pthread__mutex_lock_slow(pthread_mutex_t *ptm)
+pthread__mutex_lock_slow(pthread_mutex_t *ptm, const struct timespec *ts)
{
void *waiters, *new, *owner, *next;
pthread_t self;
int serrno;
+ int error;

pthread__error(EINVAL, "Invalid mutex",
ptm->ptm_magic == _PT_MUTEX_MAGIC);
@@ -282,6 +315,10 @@ pthread__mutex_lock_slow(pthread_mutex_t *ptm)
return EDEADLK;
}

+ /*priority protect*/
+ if (MUTEX_PROTECT(owner) && _sched_protect(ptm->ptm_ceiling) == -1) {
+ return errno;
+ }
serrno = errno;
for (;; owner = ptm->ptm_owner) {
/* Spin while the owner is running. */
@@ -339,12 +376,23 @@ pthread__mutex_lock_slow(pthread_mutex_t *ptm)
*/
while (self->pt_mutexwait) {
self->pt_blocking++;
- (void)_lwp_park(CLOCK_REALTIME, TIMER_ABSTIME, NULL,
+ error = _lwp_park(CLOCK_REALTIME, TIMER_ABSTIME, ts,
self->pt_unpark, __UNVOLATILE(&ptm->ptm_waiters),
__UNVOLATILE(&ptm->ptm_waiters));
self->pt_unpark = 0;
self->pt_blocking--;
membar_sync();
+ if (__predict_true(error != -1)) {
+ continue;
+ }
+ if (errno == ETIMEDOUT && self->pt_mutexwait) {
+ /*Remove self from waiters list*/
+ pthread__mutex_wakeup(self, ptm);
+ /*priority protect*/
+ if (MUTEX_PROTECT(owner))
+ (void)_sched_protect(-1);
+ return ETIMEDOUT;
+ }
}
}
}
@@ -460,6 +508,10 @@ pthread__mutex_unlock_slow(pthread_mutex_t *ptm)
*/
if (new != owner) {
owner = atomic_swap_ptr(&ptm->ptm_owner, new);
+ if (__predict_false(MUTEX_PROTECT(owner))) {
+ /*restore elevated priority*/
+ (void)_sched_protect(-1);
+ }
if (MUTEX_HAS_WAITERS(owner) != 0) {
pthread__mutex_wakeup(self, ptm);
return 0;
@@ -591,16 +643,21 @@ pthread_mutexattr_destroy(pthread_mutexattr_t *attr)
int
pthread_mutexattr_gettype(const pthread_mutexattr_t *attr, int *typep)
{
+ uintptr_t val;
+
pthread__error(EINVAL, "Invalid mutex attribute",
attr->ptma_magic == _PT_MUTEXATTR_MAGIC);

- *typep = (int)(intptr_t)attr->ptma_private;
+ val = (uintptr_t)attr->ptma_private & ~0x00ffL;
+ *typep = (int)val;

I'd use a constant for 0x00ff, instead of repeating it everywhere.
In fact I'd define and use accessor macros for fields setting and getting


return 0;
}

int
pthread_mutexattr_settype(pthread_mutexattr_t *attr, int type)
{
+ uintptr_t val;
+
if (__predict_false(__uselibcstub))
return __libc_mutexattr_settype_stub(attr, type);

@@ -611,7 +668,8 @@ pthread_mutexattr_settype(pthread_mutexattr_t *attr, int type)
case PTHREAD_MUTEX_NORMAL:
case PTHREAD_MUTEX_ERRORCHECK:
case PTHREAD_MUTEX_RECURSIVE:
- attr->ptma_private = (void *)(intptr_t)type;
+ val = (uintptr_t)attr->ptma_private & ~0x00ffL;
+ attr->ptma_private = (void*)(val | type);
return 0;
default:
return EINVAL;
@@ -619,6 +677,67 @@ pthread_mutexattr_settype(pthread_mutexattr_t *attr, int type)
}

int
+pthread_mutexattr_getprotocol(const pthread_mutexattr_t *attr, int*proto)
+{
+ uintptr_t val;
+
+ pthread__error(EINVAL, "Invalid mutex attribute",
+ attr->ptma_magic == _PT_MUTEXATTR_MAGIC);
+
+ val = (uintptr_t)attr->ptma_private & ~0xff00L;
+ *proto = (int)(val >> 8);
+ return 0;
+}
+
+int
+pthread_mutexattr_setprotocol(pthread_mutexattr_t* attr, int proto)
+{
+ uintptr_t val;
+
+ pthread__error(EINVAL, "Invalid mutex attribute",
+ attr->ptma_magic == _PT_MUTEXATTR_MAGIC);
+
+ switch (proto) {
+ case PTHREAD_PRIO_NONE:
+ case PTHREAD_PRIO_PROTECT:
+ val = (uintptr_t)attr->ptma_private & ~0xff00L;
+ attr->ptma_private = (void *)(val | (proto << 8));
+ return 0;
+ case PTHREAD_PRIO_INHERIT:
+ return ENOTSUP;
+ default:
+ return EINVAL;
+ }
+}
+
+int
+pthread_mutexattr_getprioceiling(const pthread_mutexattr_t *attr, int *ceil)
+{
+ uintptr_t val;
+
+ pthread__error(EINVAL, "Invalid mutex attribute",
+ attr->ptma_magic == _PT_MUTEXATTR_MAGIC);
+
+ val = (uintptr_t)attr->ptma_private & ~0xff0000L;
+ *ceil = (int)(val >> 16);
+ return 0;
+}
+
+int
+pthread_mutexattr_setprioceiling(pthread_mutexattr_t *attr, int ceil)
+{
+ uintptr_t val;
+
+ pthread__error(EINVAL, "Invalid mutex attribute",
+ attr->ptma_magic == _PT_MUTEXATTR_MAGIC);

Indent, second level indent is 4 spaces

+
+ /*check range*/
+ val = (uintptr_t)attr->ptma_private & ~0xff0000L;
+ attr->ptma_private = (void*)(val | (ceil << 16));
+ return 0;
+}
+
+int
pthread_mutexattr_getpshared(const pthread_mutexattr_t *__restrict attr, int* __restrict pshared) {
*pshared = PTHREAD_PROCESS_PRIVATE;
return 0;
@@ -662,6 +781,28 @@ pthread__mutex_deferwake(pthread_t self, pthread_mutex_t *ptm)
}

int
+pthread_mutex_getprioceiling(const pthread_mutex_t *ptm, int*ceil)
+{
+ *ceil = (unsigned int)ptm->ptm_ceiling;
+ return 0;
+}
+
+int
+pthread_mutex_setprioceiling(pthread_mutex_t *ptm, int ceil, int *old_ceil)
+{
+ int error;
+
+ error = pthread_mutex_lock(ptm);
+ if (error == 0) {
+ *old_ceil = (unsigned int)ptm->ptm_ceiling;
+ /*check range*/
+ ptm->ptm_ceiling = (unsigned char)ceil;
+ pthread_mutex_unlock(ptm);
+ }
+ return error;
+}
+
+int
_pthread_mutex_held_np(pthread_mutex_t *ptm)
{

diff --git a/lib/libpthread/pthread_types.h b/lib/libpthread/pthread_types.h
index a25be8e..7981348 100644
--- a/lib/libpthread/pthread_types.h
+++ b/lib/libpthread/pthread_types.h
@@ -115,7 +115,7 @@ struct __pthread_mutex_st {
#ifdef __CPU_SIMPLE_LOCK_PAD
uint8_t ptm_pad1[3];
#endif
- __pthread_spin_t ptm_interlock; /* unused - backwards compat */
+ __pthread_spin_t ptm_ceiling;
#ifdef __CPU_SIMPLE_LOCK_PAD
uint8_t ptm_pad2[3];
#endif
@@ -130,14 +130,14 @@ struct __pthread_mutex_st {

#ifdef __CPU_SIMPLE_LOCK_PAD
#define _PTHREAD_MUTEX_INITIALIZER { _PT_MUTEX_MAGIC, \
- __SIMPLELOCK_UNLOCKED, { 0, 0, 0 }, \
+ __SIMPLELOCK_UNLOCKED, { 0, 0, 0 }, \
__SIMPLELOCK_UNLOCKED, { 0, 0, 0 }, \
NULL, NULL, 0, NULL \
}
#else
#define _PTHREAD_MUTEX_INITIALIZER { _PT_MUTEX_MAGIC, \
- __SIMPLELOCK_UNLOCKED, \
- __SIMPLELOCK_UNLOCKED, \
+ __SIMPLELOCK_UNLOCKED, \
+ __SIMPLELOCK_UNLOCKED, \
NULL, NULL, 0, NULL \
}
These were not changed, why are there whitespace diffs?

christos

Christos Zoulas

unread,
May 30, 2016, 4:06:16 PM5/30/16
to
On May 30, 10:33am, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

The bsd_signal patch looks good!

christos

Martin Husemann

unread,
May 30, 2016, 4:06:31 PM5/30/16
to
On Mon, May 30, 2016 at 03:34:20PM -0400, Christos Zoulas wrote:
> It is not a function pointer; it is a function that takes as an argument
> the new signal handler (function pointer) returns a function
> pointer (the old signal handler function)

Duh, wrong paranthization - I'll shut up.

Martin

Martin Husemann

unread,
May 30, 2016, 4:30:35 PM5/30/16
to
On Mon, May 30, 2016 at 01:25:34PM -0700, Charles Cui wrote:
> there is one remaining slot. And each of them will increase p_nsem by one
> using atomic_inc, the results are we have 1 slot overused, but the error is
> never detected.

You will have to use a _nv() variant and use the return value to
compare that against the limit, then decrement in the error path.

Martin

Christos Zoulas

unread,
May 30, 2016, 4:46:56 PM5/30/16
to
On May 30, 1:43pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

| I am not familiar with _nv() (are there examples in the netbsd code base? I
| searched _nv() in nxr, but did not found anything meaningful.),
| but if you want to use atomic operations, one possible way is
| compare-and-swap (CAS).
| it has stronger guarantee than atomic inc or atomic dec, but also larger
| overhead.

man atomic_inc_uint_nv

http://nxr.netbsd.org/xref/src/sys/fs/tmpfs/tmpfs_mem.c#185

christos

Charles Cui

unread,
May 30, 2016, 5:11:42 PM5/30/16
to
Hi Kamil,

_SC_SEM_NSEMS_MAX is not the same with SEM_VALUE_MAX.
The first one is used to limit the number of sems for a process, while
the second is used to set the initial value for a semaphore.

Charles Cui

unread,
May 31, 2016, 5:34:10 AM5/31/16
to
No, using atomic_inc / atomic_dec for p_nsem is wrong!

Let's think about the multithreading case. Let's use 2 threads A and B in
the same process as an example. Assume p_nsem is not reaching the limit,
but only one slot is left.
A and B will each read the p_nsem separately. There will be probability
that they both find
there is one remaining slot. And each of them will increase p_nsem by one
using atomic_inc, the results are we have 1 slot overused, but the error is
never detected.


2016-05-30 12:28 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On May 30, 8:29pm, mar...@duskware.de (Martin Husemann) wrote:
> -- Subject: Re: pthread library related
>
> | Do we really need this? Why is it a function pointer?
>
> It is the same as signal.
>

Charles Cui

unread,
May 31, 2016, 5:34:17 AM5/31/16
to
I am not familiar with _nv() (are there examples in the netbsd code base? I
searched _nv() in nxr, but did not found anything meaningful.),
but if you want to use atomic operations, one possible way is
compare-and-swap (CAS).
it has stronger guarantee than atomic inc or atomic dec, but also larger
overhead.

2016-05-30 13:30 GMT-07:00 Martin Husemann <mar...@duskware.de>:

> On Mon, May 30, 2016 at 01:25:34PM -0700, Charles Cui wrote:
> > there is one remaining slot. And each of them will increase p_nsem by one
> > using atomic_inc, the results are we have 1 slot overused, but the error
> is
> > never detected.
>

Charles Cui

unread,
May 31, 2016, 5:49:43 AM5/31/16
to
good to know atomic_inc_uint_nv is implemented using cas.
I will update the patch and send you guys later (being careful about the
format)

Thanks Charles

2016-05-30 13:46 GMT-07:00 Christos Zoulas <chri...@zoulas.com>:

> On May 30, 1:43pm, charles...@gmail.com (Charles Cui) wrote:
> -- Subject: Re: pthread library related
>
> | I am not familiar with _nv() (are there examples in the netbsd code
> base? I
> | searched _nv() in nxr, but did not found anything meaningful.),
> | but if you want to use atomic operations, one possible way is
> | compare-and-swap (CAS).
> | it has stronger guarantee than atomic inc or atomic dec, but also larger
> | overhead.
>

Joerg Sonnenberger

unread,
May 31, 2016, 7:26:36 AM5/31/16
to
On Mon, May 30, 2016 at 02:19:26PM -0700, Charles Cui wrote:
> good to know atomic_inc_uint_nv is implemented using cas.

No, atomic_inc is *not* necessarily implemented using CAS. There are a
couple of different ways to do it ranging from implicit serialisation on
UP-only systems over CAS/LL-SC loops to native locked inc instructions.
For the application program, it doesn't generally matter which it is.

Joerg

Steffen Nurpmeso

unread,
May 31, 2016, 10:47:42 AM5/31/16
to
Joerg Sonnenberger <jo...@bec.de> wrote:
|On Mon, May 30, 2016 at 02:19:26PM -0700, Charles Cui wrote:
|> good to know atomic_inc_uint_nv is implemented using cas.
|
|No, atomic_inc is *not* necessarily implemented using CAS. There are a

What a pity.

|couple of different ways to do it ranging from implicit serialisation on
|UP-only systems over CAS/LL-SC loops to native locked inc instructions.
|For the application program, it doesn't generally matter which it is.

Remembering some atomic integer (inline) functions seen in the
Linux Kernel (Alpha?) before discovering FreeBSD (v4.7), i'd
rather go with cas (or even locked xchg if not otherwise possible)
spinlocks (or noops without SMP) and a normal integer increment
instead. You can even PAUSE. That was a good hardware addition.
(Not that i have any idea of hardware.)

--steffen

Martin Husemann

unread,
May 31, 2016, 10:57:52 AM5/31/16
to
On Tue, May 31, 2016 at 04:49:00AM +0200, Steffen Nurpmeso wrote:
> Joerg Sonnenberger <jo...@bec.de> wrote:
> |On Mon, May 30, 2016 at 02:19:26PM -0700, Charles Cui wrote:
> |> good to know atomic_inc_uint_nv is implemented using cas.
> |
> |No, atomic_inc is *not* necessarily implemented using CAS. There are a
>
> What a pity.

Not at all.

Read it again: (a) the application [or kernel code] should not care about
the implementation details and (b) it MAY be implemented differently - the
decision is up to the architecture and depends on various things. As Jörg
said, sometimes the implementation will be different depending on the number
of processors or exact cpu (sub-)version found.

On some architectures (like m68k, where we do not even support SMP) it will
be a simple pre-increment/decrement instruction, no overhead at all.

Martin

Charles Cui

unread,
Jun 3, 2016, 1:02:19 AM6/3/16
to
If there are no comments for these two patches.
I will move forward for other parts.

Sounds good?

2016-06-01 19:28 GMT-07:00 Charles Cui <charles...@gmail.com>:

> Hi guys,
>
> I have attached patches based on comments from Christos.
> The new patch used atomic_inc_uint_nv to limit the number of
> sems used by each process. I have tested these patches.
> They worked.
>
> 2016-05-30 14:19 GMT-07:00 Charles Cui <charles...@gmail.com>:
>
>> good to know atomic_inc_uint_nv is implemented using cas.

Christos Zoulas

unread,
Jun 3, 2016, 4:49:03 PM6/3/16
to
On Jun 2, 5:41pm, charles...@gmail.com (Charles Cui) wrote:
-- Subject: Re: pthread library related

| If there are no comments for these two patches.
| I will move forward for other parts.
|
| Sounds good?
|

Have you created a github repository for the patches and one for
the changes to the posix tests?

christos

0 new messages