Re: [9front] Re: [Patch] APE changes (2019 Lufia patches)

[ori@eigenstate.org]

Quoth bsdsm@sdf.org:
> 
> Sorry for the delay. Attached are the changes, minus architecture
> specific code, with interests separated as previously discussed.
> 
> Please allow me a moment to address my intentions. My goals are more
> human than software related. I began working on a port of Lufia's
> changes not long after the pull requests. I stopped for some of the
> reasons already discussed: git9, libressl isn't desired, etc. I
> decided to put something together only after finding out that those
> original patches are still in limbo after two years through 9fans. My
> intention being that if even a single line presented was thought
> worthy of import then I could reach out to Lufia and say, 'hey check
> this out, and maybe your next patch can come straight to 9front.'
> 
> Perhaps my goals were misguided. It is not my intention to denigrate
> anyone, 9fan or otherwise.
> 
> As a final note I agree with the comments made previously about
> nitpicking the changes. Please nitpick everything. I just wanted to
> make my intentions clearer.

Alright, finally started looking over the pthreads patch,
since it's the most interesting.

> 
> diff -r bfe93397b157 sys/include/ape/pthread.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/sys/include/ape/pthread.h	Fri Feb 12 10:59:32 2021 -0700
> @@ -0,0 +1,105 @@
> +#ifndef __PTHREAD_H
> +#define __PTHREAD_H
> +#pragma lib "/$M/lib/ape/libpthread.a"
> +
> +#define _LOCK_EXTENSION
> +#define _QLOCK_EXTENSION
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <lock.h>
> +#include <qlock.h>

We need to be careful here -- this litters the namespace,
in a way that it strictly shouldn't. the types that get
dragged in should, in theory, get prefixed by an '_' or
'__' to prevent namespace clashes.

> +
> +typedef struct pthread_once pthread_once_t;
> +typedef pid_t pthread_t;
> +typedef int pthread_attr_t;
> +typedef struct pthread_mutex pthread_mutex_t;
> +typedef int pthread_mutexattr_t;
> +typedef struct pthread_cond pthread_cond_t;
> +typedef int pthread_condattr_t;
> +typedef struct pthread_key pthread_key_t;
> +
> +enum {
> +	PTHREAD_THREADS_MAX = 1000,

This is an odd limit. As far as I can tell, this is entirely
due to the privates array -- if we move the array contents
into the pthread itself, we can remove the limit.

> +
> +	PTHREAD_CANCEL_DISABLE = 1,
> +};
> +
> +struct pthread_once {
> +	Lock l;
> +	int once;
> +};
> +struct pthread_mutex {
> +	QLock l;
> +
> +	Lock mu;
> +	pthread_t pid;
> +	int ref;
> +	pthread_mutexattr_t attr;
> +};
> +struct pthread_cond {
> +	QLock l;
> +	Rendez r;
> +};
> +struct pthread_key {
> +	Lock l;
> +	void (*destroy)(void*);
> +	struct {
> +		pthread_t	pid;
> +		const void	*p;
> +	} *arenas;
> +	int n;
> +};
> +
> +#define PTHREAD_ONCE_INIT		{ 0 }
> +#define PTHREAD_MUTEX_INITIALIZER	{ 0 }
> +#define PTHREAD_MUTEX_DEFAULT		0
> +#define PTHREAD_MUTEX_NORAML		1

Typo: NORMAL. We should also use an enum here,
and

	#define PTHREAD_MUTEX_DEFAULT PTHREAD_MUTEX_NORMAL

> +#define PTHREAD_MUTEX_RECURSIVE	2
> +#define PTHREAD_COND_INITIALIZER	{ 0 }
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern int	pthread_atfork(void (*)(void), void (*)(void), void (*)(void));
> +extern int	pthread_once(pthread_once_t*, void (*)(void));
> +extern pthread_t	pthread_self(void);
> +extern int	pthread_equal(pthread_t, pthread_t);
> +extern int	pthread_create(pthread_t*, pthread_attr_t*, void *(*)(void*), void*);
> +extern void	pthread_exit(void*);
> +extern int	pthread_join(pthread_t, void**);
> +
> +extern int	pthread_mutexattr_init(pthread_mutexattr_t*);
> +extern int	pthread_mutexattr_destroy(pthread_mutexattr_t*);
> +extern int	pthread_mutexattr_settype(pthread_mutexattr_t*, int);
> +
> +extern int	pthread_mutex_init(pthread_mutex_t*, const pthread_mutexattr_t*);
> +extern int	pthread_mutex_lock(pthread_mutex_t*);
> +extern int	pthread_mutex_unlock(pthread_mutex_t*);
> +extern int	pthread_mutex_trylock(pthread_mutex_t*);
> +extern int	pthread_mutex_destroy(pthread_mutex_t*);
> +
> +extern int	pthread_cond_init(pthread_cond_t*, pthread_condattr_t*);
> +extern int	pthread_cond_wait(pthread_cond_t*, pthread_mutex_t*);
> +extern int	pthread_cond_signal(pthread_cond_t*);
> +extern int	pthread_cond_broadcast(pthread_cond_t*);
> +extern int	pthread_cond_destroy(pthread_cond_t*);
> +
> +extern int	pthread_key_create(pthread_key_t*, void (*)(void*));
> +extern int	pthread_key_delete(pthread_key_t);
> +extern void	*pthread_getspecific(pthread_key_t);
> +extern int	pthread_setspecific(pthread_key_t, const void*);
> +
> +extern int	pthread_setcancelstate(int, int*);
> +
> +#ifndef _PTHREAD_SIGMASK
> +#define _PTHREAD_SIGMASK
> +#include <signal.h>
> +extern int	pthread_sigmask(int, const sigset_t*, sigset_t*);
> +#endif
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif
> diff -r bfe93397b157 sys/include/ape/qlock.h
> --- a/sys/include/ape/qlock.h	Wed Sep 16 20:45:49 2020 +0000
> +++ b/sys/include/ape/qlock.h	Fri Feb 12 10:59:32 2021 -0700

To get the namespacing, we should get rid of the _LOCK_EXTENSION
define from pthread.; instead, we should do something like:

	typedef struct _Rendez _Rendez;
	#ifdef _LOCK_EXTENSION
	typedef _Rendez Rendez;
	#endif

> @@ -26,6 +26,14 @@
>  	QLp 	*tail;
>  } QLock;
>  
> +typedef
> +struct Rendez
> +{
> +	QLock	*l;
> +	QLp	*head;
> +	QLp	*tail;
> +} Rendez;
> +
>  #ifdef __cplusplus
>  extern "C" {
>  #endif
> @@ -34,6 +42,10 @@
>  extern	void	qunlock(QLock*);
>  extern	int	canqlock(QLock*);
>  
> +extern	void	rsleep(Rendez*);
> +extern	int	rwakeup(Rendez*);
> +extern	int	rwakeupall(Rendez*);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff -r 02e3059af5bc -r 5904c2b92376 sys/src/ape/lib/ap/plan9/qlock.c
> --- a/sys/src/ape/lib/ap/plan9/qlock.c	Thu Feb 11 09:37:36 2021 +0100
> +++ b/sys/src/ape/lib/ap/plan9/qlock.c	Thu Oct 26 02:42:26 2017 +0200
> @@ -22,6 +22,9 @@
>  enum
>  {
>  	Queuing,
> +	QueuingR,
> +	QueuingW,
> +	Sleeping,
>  };
>  
>  /* find a free shared memory location to queue ourselves in */
> @@ -108,3 +112,254 @@
>  	unlock(&q->lock);
>  	return 0;
>  }
> +
> +#if 0

Dead code should be deleted.

> +void
> +rlock(RWLock *q)
> +{
> +	QLp *p, *mp;
> +
> +	lock(&q->lock);
> +	if(q->writer == 0 && q->head == nil){
> +		/* no writer, go for it */
> +		q->readers++;
> +		unlock(&q->lock);
> +		return;
> +	}
> +
> +	mp = getqlp();
> +	p = q->tail;
> +	if(p == 0)
> +		q->head = mp;
> +	else
> +		p->next = mp;
> +	q->tail = mp;
> +	mp->next = nil;
> +	mp->state = QueuingR;
> +	unlock(&q->lock);
> +
> +	/* wait in kernel */
> +	while((*_rendezvousp)(mp, (void*)1) == (void*)~0)
> +		;
> +	mp->inuse = 0;
> +}
> +
> +int
> +canrlock(RWLock *q)
> +{
> +	lock(&q->lock);
> +	if (q->writer == 0 && q->head == nil) {
> +		/* no writer; go for it */
> +		q->readers++;
> +		unlock(&q->lock);
> +		return 1;
> +	}
> +	unlock(&q->lock);
> +	return 0;
> +}
> +
> +void
> +runlock(RWLock *q)
> +{
> +	QLp *p;
> +
> +	lock(&q->lock);
> +	if(q->readers <= 0)
> +		abort();
> +	p = q->head;
> +	if(--(q->readers) > 0 || p == nil){
> +		unlock(&q->lock);
> +		return;
> +	}
> +
> +	/* start waiting writer */
> +	if(p->state != QueuingW)
> +		abort();
> +	q->head = p->next;
> +	if(q->head == 0)
> +		q->tail = 0;
> +	q->writer = 1;
> +	unlock(&q->lock);
> +
> +	/* wakeup waiter */
> +	while((*_rendezvousp)(p, (void*)0) == (void*)~0)
> +		;
> +}
> +
> +void
> +wlock(RWLock *q)
> +{
> +	QLp *p, *mp;
> +
> +	lock(&q->lock);
> +	if(q->readers == 0 && q->writer == 0){
> +		/* noone waiting, go for it */
> +		q->writer = 1;
> +		unlock(&q->lock);
> +		return;
> +	}
> +
> +	/* wait */
> +	p = q->tail;
> +	mp = getqlp();
> +	if(p == nil)
> +		q->head = mp;
> +	else
> +		p->next = mp;
> +	q->tail = mp;
> +	mp->next = nil;
> +	mp->state = QueuingW;
> +	unlock(&q->lock);
> +
> +	/* wait in kernel */
> +	while((*_rendezvousp)(mp, (void*)1) == (void*)~0)
> +		;
> +	mp->inuse = 0;
> +}
> +
> +int
> +canwlock(RWLock *q)
> +{
> +	lock(&q->lock);
> +	if (q->readers == 0 && q->writer == 0) {
> +		/* no one waiting; go for it */
> +		q->writer = 1;
> +		unlock(&q->lock);
> +		return 1;
> +	}
> +	unlock(&q->lock);
> +	return 0;
> +}
> +
> +void
> +wunlock(RWLock *q)
> +{
> +	QLp *p;
> +
> +	lock(&q->lock);
> +	if(q->writer == 0)
> +		abort();
> +	p = q->head;
> +	if(p == nil){
> +		q->writer = 0;
> +		unlock(&q->lock);
> +		return;
> +	}
> +	if(p->state == QueuingW){
> +		/* start waiting writer */
> +		q->head = p->next;
> +		if(q->head == nil)
> +			q->tail = nil;
> +		unlock(&q->lock);
> +		while((*_rendezvousp)(p, (void*)0) == (void*)~0)
> +			;
> +		return;
> +	}
> +
> +	if(p->state != QueuingR)
> +		abort();
> +
> +	/* wake waiting readers */
> +	while(q->head != nil && q->head->state == QueuingR){
> +		p = q->head;
> +		q->head = p->next;
> +		q->readers++;
> +		while((*_rendezvousp)(p, (void*)0) == (void*)~0)
> +			;
> +	}
> +	if(q->head == nil)
> +		q->tail = nil;
> +	q->writer = 0;
> +	unlock(&q->lock);
> +}
> +
> +void
> +rsleep(Rendez *r)
> +{
> +	QLp *t, *me;
> +
> +	if(!r->l)
> +		abort();
> +	lock(&r->l->lock);
> +	/* we should hold the qlock */
> +	if(!r->l->locked)
> +		abort();
> +
> +	/* add ourselves to the wait list */
> +	me = getqlp();
> +	me->state = Sleeping;
> +	if(r->head == nil)
> +		r->head = me;
> +	else
> +		r->tail->next = me;
> +	me->next = nil;
> +	r->tail = me;
> +
> +	/* pass the qlock to the next guy */
> +	t = r->l->head;
> +	if(t){
> +		r->l->head = t->next;
> +		if(r->l->head == nil)
> +			r->l->tail = nil;
> +		unlock(&r->l->lock);
> +		while((*_rendezvousp)(t, (void*)0x12345) == (void*)~0)
> +			;
> +	}else{
> +		r->l->locked = 0;
> +		unlock(&r->l->lock);
> +	}
> +
> +	/* wait for a wakeup */
> +	while((*_rendezvousp)(me, (void*)1) == (void*)~0)
> +		;
> +	me->inuse = 0;
> +}
> +
> +int
> +rwakeup(Rendez *r)
> +{
> +	QLp *t;
> +
> +	/*
> +	 * take off wait and put on front of queue
> +	 * put on front so guys that have been waiting will not get starved
> +	 */
> +	
> +	if(!r->l)
> +		abort();
> +	lock(&r->l->lock);
> +	if(!r->l->locked)
> +		abort();
> +
> +	t = r->head;
> +	if(t == nil){
> +		unlock(&r->l->lock);
> +		return 0;
> +	}
> +
> +	r->head = t->next;
> +	if(r->head == nil)
> +		r->tail = nil;
> +
> +	t->next = r->l->head;
> +	r->l->head = t;
> +	if(r->l->tail == nil)
> +		r->l->tail = t;
> +
> +	t->state = Queuing;
> +	unlock(&r->l->lock);
> +	return 1;
> +}
> +
> +int
> +rwakeupall(Rendez *r)
> +{
> +	int i;
> +
> +	for(i=0; rwakeup(r); i++)
> +		;
> +	return i;
> +}
> +
> +#endif
> diff -r bfe93397b157 sys/src/ape/lib/ap/plan9/fork.c
> --- a/sys/src/ape/lib/ap/plan9/fork.c	Wed Sep 16 20:45:49 2020 +0000
> +++ b/sys/src/ape/lib/ap/plan9/fork.c	Fri Feb 12 10:59:32 2021 -0700
> @@ -2,18 +2,58 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include "sys9.h"
> +#include <pthread.h>
> +
> +enum {
> +	NHANDLERS = 100
> +};
> +
> +static void (*preparehdlr[NHANDLERS])(void);
> +static void (*parenthdlr[NHANDLERS])(void);
> +static void (*childhdlr[NHANDLERS])(void);
> +static int nprepare;
> +static int nparent;
> +static int nchild;
> +
> +int
> +pthread_atfork(void (*prepare)(void), void (*parent)(void), void (*child)(void))
> +{

I think we need a lock to protect these handlers.

> +	if(prepare != NULL){
> +		if(nprepare >= NHANDLERS)
> +			return ENOMEM;
> +		preparehdlr[nprepare++] = prepare;
> +	}
> +	if(parent != NULL){
> +		if(nparent >= NHANDLERS)
> +			return ENOMEM;
> +		parenthdlr[nparent++] = parent;
> +	}
> +	if(child != NULL){
> +		if(nchild >= NHANDLERS)
> +			return ENOMEM;
> +		childhdlr[nchild++] = child;
> +	}
> +	return 0;
> +}
>  
>  pid_t
>  fork(void)
>  {
> -	int n;
> +	int n, i;
>  
> +	for(i = nprepare-1; i >= 0; i--)
> +		preparehdlr[i]();
>  	n = _RFORK(RFENVG|RFFDG|RFPROC);

Need to be careful -- are there any other places that can fork?
system()?

>  	if(n < 0)
>  		_syserrno();
>  	if(n == 0) {
>  		_detachbuf();
>  		_sessleader = 0;
> +		for(i = 0; i < nchild; i++)
> +			childhdlr[i]();
> +		return 0;
>  	}
> +	for(i = 0; i < nparent; i++)
> +		parenthdlr[i]();
>  	return n;
>  }
> diff -r bfe93397b157 sys/src/ape/lib/pthread/_pthread.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/sys/src/ape/lib/pthread/_pthread.c	Fri Feb 12 10:59:32 2021 -0700
> @@ -0,0 +1,83 @@
> +#include <pthread.h>
> +#include <string.h>
> +#include "lib.h"
> +
> +static Lock privlock;
> +static Thread privileges[PTHREAD_THREADS_MAX];

I think that 'privileges' is a misunderstanding of the
'priv' abbreviation -- I think we want 'privates' here.
I also don't think this should be a global array, but
something that we put into the pthread_t itself.

	typedef Thread* pthread_t;

We can even use privalloc() here, but I don't think
we need to.

I think this is an invasive enough change that I can
do another round after it gets done.

> +pthread_join(pthread_t t, void **ret)
> +{
> +	static Lock l;
> +	Thread *priv;
> +	int pid;
> +	Waitmsg *msg;
> +
> +	lock(&l);
> +	priv = _pthreadget(t);
> +	if(priv == NULL){
> +		unlock(&l);
> +		return EINVAL;
> +	}
> +	lock(&priv->l);
> +	if(priv->exited){
> +		emitexits(ret, priv);
> +		unlock(&priv->l);
> +		_pthreadfree(priv);
> +		unlock(&l);
> +		return 0;
> +	}
> +	unlock(&priv->l);
> +
> +	while((msg=_wait()) != NULL){
> +		pid = msg->pid;
> +		free(msg);
> +		if(pid == t)
> +			break;
> +	}

This loop is wrong -- if the threads exit out of
order, then we lose the wait messages and never
join. We'll probably need to set up some sort of
wait notification dance -- possibly an exit cond
var for non-detached threads.

> +	lock(&priv->l);
> +	if(priv->exited){
> +		emitexits(ret, priv);
> +		unlock(&priv->l);
> +		_pthreadfree(priv);
> +		unlock(&l);
> +		return 0;
> +	}
> +	unlock(&priv->l);
> +	unlock(&l);
> +	return ESRCH;
> +}

> diff -r bfe93397b157 sys/src/ape/lib/pthread/mutex_unlock.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/sys/src/ape/lib/pthread/mutex_unlock.c	Fri Feb 12 10:59:32 2021 -0700
> @@ -0,0 +1,29 @@
> +#include <pthread.h>
> +#include <errno.h>
> +
> +int
> +pthread_mutex_unlock(pthread_mutex_t *mutex)
> +{
> +	pthread_t pid;
> +
> +	pid = pthread_self();
> +	lock(&mutex->mu);
> +	if(mutex->pid != pid){
> +		unlock(&mutex->mu);
> +		return EPERM;
> +	}
> +	if(mutex->attr == PTHREAD_MUTEX_RECURSIVE){
> +		mutex->ref--;
> +		if(mutex->ref <= 0){

Abort if it's less than zero -- let's not paper over
programmer issues.

> +			mutex->pid = 0;
> +			mutex->ref = 0;
> +			qunlock(&mutex->l);
> +		}
> +		unlock(&mutex->mu);
> +		return 0;
> +	}
> +	mutex->ref--;
> +	qunlock(&mutex->l);
> +	unlock(&mutex->mu);
> +	return 0;
> diff -r bfe93397b157 sys/src/ape/lib/pthread/self.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/sys/src/ape/lib/pthread/self.c	Fri Feb 12 10:59:32 2021 -0700
> @@ -0,0 +1,8 @@
> +#include <pthread.h>
> +#include <unistd.h>
> +
> +pthread_t
> +pthread_self(void)
> +{
> +	return getpid();

Obviously, this would need to change to use
privalloc() or similar once we get rid of the
priv array.