git: a5c2f4e93943 - main - libc/libc/rpc: refactor some global variables

From: Alan Somers <asomers_at_FreeBSD.org>
Date: Wed, 15 Nov 2023 23:13:10 UTC
The branch main has been updated by asomers:

URL: https://cgit.FreeBSD.org/src/commit/?id=a5c2f4e939430f0048136c39fb9fa6093d401905

commit a5c2f4e939430f0048136c39fb9fa6093d401905
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2023-11-09 22:58:56 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2023-11-15 23:12:50 +0000

    libc/libc/rpc: refactor some global variables
    
    * Combine dg_fd_locks and dg_cv into one array.
    * Similarly for vc_fd_locks and vc_cv
    * Turn some macros into inline functions
    
    This is a mostly cosmetic change to make refactoring these strutures in
    a future commit easier.
    
    MFC after:      2 weeks
    Sponsored by:   Axcient
    Reviewed by:    kib
    Differential Revision: https://reviews.freebsd.org/D42597
---
 lib/libc/rpc/clnt_dg.c | 93 +++++++++++++++++++++++---------------------------
 lib/libc/rpc/clnt_vc.c | 92 ++++++++++++++++++++++---------------------------
 2 files changed, 83 insertions(+), 102 deletions(-)

diff --git a/lib/libc/rpc/clnt_dg.c b/lib/libc/rpc/clnt_dg.c
index 40511f30135e..7f741f932c42 100644
--- a/lib/libc/rpc/clnt_dg.c
+++ b/lib/libc/rpc/clnt_dg.c
@@ -89,28 +89,31 @@ static void clnt_dg_destroy(CLIENT *);
  *	This machinery implements per-fd locks for MT-safety.  It is not
  *	sufficient to do per-CLIENT handle locks for MT-safety because a
  *	user may create more than one CLIENT handle with the same fd behind
- *	it.  Therefore, we allocate an array of flags (dg_fd_locks), protected
- *	by the clnt_fd_lock mutex, and an array (dg_cv) of condition variables
- *	similarly protected.  Dg_fd_lock[fd] == 1 => a call is active on some
- *	CLIENT handle created for that fd.
- *	The current implementation holds locks across the entire RPC and reply,
- *	including retransmissions.  Yes, this is silly, and as soon as this
- *	code is proven to work, this should be the first thing fixed.  One step
- *	at a time.
+ *	it.  Therefore, we allocate an array of flags and condition variables
+ *	(dg_fd) protected by the clnt_fd_lock mutex.  dg_fd[fd].lock == 1 => a
+ *	call is active on some CLIENT handle created for that fd.  The current
+ *	implementation holds locks across the entire RPC and reply, including
+ *	retransmissions.  Yes, this is silly, and as soon as this code is
+ *	proven to work, this should be the first thing fixed.  One step at a
+ *	time.
  */
-static int	*dg_fd_locks;
-static cond_t	*dg_cv;
-#define	release_fd_lock(fd, mask) {		\
-	mutex_lock(&clnt_fd_lock);	\
-	dg_fd_locks[fd] = 0;		\
-	mutex_unlock(&clnt_fd_lock);	\
-	thr_sigsetmask(SIG_SETMASK, &(mask), NULL); \
-	cond_signal(&dg_cv[fd]);	\
+static struct {
+	int lock;
+	cond_t cv;
+} *dg_fd;
+static void
+release_fd_lock(int fd, sigset_t mask)
+{
+	mutex_lock(&clnt_fd_lock);
+	dg_fd[fd].lock = 0;
+	mutex_unlock(&clnt_fd_lock);
+	thr_sigsetmask(SIG_SETMASK, &mask, NULL);
+	cond_signal(&dg_fd[fd].cv);
 }
 
 static const char mem_err_clnt_dg[] = "clnt_dg_create: out of memory";
 
-/* VARIABLES PROTECTED BY clnt_fd_lock: dg_fd_locks, dg_cv */
+/* VARIABLES PROTECTED BY clnt_fd_lock: dg_fd */
 
 #define	MCALL_MSG_SIZE 24
 
@@ -176,34 +179,22 @@ clnt_dg_create(int fd, const struct netbuf *svcaddr, rpcprog_t program,
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	if (dg_fd_locks == (int *) NULL) {
-		int cv_allocsz;
-		size_t fd_allocsz;
+	if (dg_fd == NULL) {
+		size_t allocsz;
+		int i;
 		int dtbsize = __rpc_dtbsize();
 
-		fd_allocsz = dtbsize * sizeof (int);
-		dg_fd_locks = (int *) mem_alloc(fd_allocsz);
-		if (dg_fd_locks == (int *) NULL) {
+		allocsz = dtbsize * sizeof (dg_fd[0]);
+		dg_fd = mem_alloc(allocsz);
+		if (dg_fd == NULL) {
 			mutex_unlock(&clnt_fd_lock);
 			thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
 			goto err1;
-		} else
-			memset(dg_fd_locks, '\0', fd_allocsz);
-
-		cv_allocsz = dtbsize * sizeof (cond_t);
-		dg_cv = (cond_t *) mem_alloc(cv_allocsz);
-		if (dg_cv == (cond_t *) NULL) {
-			mem_free(dg_fd_locks, fd_allocsz);
-			dg_fd_locks = (int *) NULL;
-			mutex_unlock(&clnt_fd_lock);
-			thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
-			goto err1;
-		} else {
-			int i;
-
-			for (i = 0; i < dtbsize; i++)
-				cond_init(&dg_cv[i], 0, (void *) 0);
 		}
+		memset(dg_fd, '\0', allocsz);
+
+		for (i = 0; i < dtbsize; i++)
+			cond_init(&dg_fd[i].cv, 0, (void *) 0);
 	}
 
 	mutex_unlock(&clnt_fd_lock);
@@ -340,13 +331,13 @@ clnt_dg_call(CLIENT *cl, rpcproc_t proc, xdrproc_t xargs, void *argsp,
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (dg_fd_locks[cu->cu_fd])
-		cond_wait(&dg_cv[cu->cu_fd], &clnt_fd_lock);
+	while (dg_fd[cu->cu_fd].lock)
+		cond_wait(&dg_fd[cu->cu_fd].cv, &clnt_fd_lock);
 	if (__isthreaded)
 		rpc_lock_value = 1;
 	else
 		rpc_lock_value = 0;
-	dg_fd_locks[cu->cu_fd] = rpc_lock_value;
+	dg_fd[cu->cu_fd].lock = rpc_lock_value;
 	mutex_unlock(&clnt_fd_lock);
 	if (cu->cu_total.tv_usec == -1) {
 		timeout = utimeout;	/* use supplied timeout */
@@ -625,13 +616,13 @@ clnt_dg_freeres(CLIENT *cl, xdrproc_t xdr_res, void *res_ptr)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (dg_fd_locks[cu->cu_fd])
-		cond_wait(&dg_cv[cu->cu_fd], &clnt_fd_lock);
+	while (dg_fd[cu->cu_fd].lock)
+		cond_wait(&dg_fd[cu->cu_fd].cv, &clnt_fd_lock);
 	xdrs->x_op = XDR_FREE;
 	dummy = (*xdr_res)(xdrs, res_ptr);
 	mutex_unlock(&clnt_fd_lock);
 	thr_sigsetmask(SIG_SETMASK, &mask, NULL);
-	cond_signal(&dg_cv[cu->cu_fd]);
+	cond_signal(&dg_fd[cu->cu_fd].cv);
 	return (dummy);
 }
 
@@ -653,13 +644,13 @@ clnt_dg_control(CLIENT *cl, u_int request, void *info)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (dg_fd_locks[cu->cu_fd])
-		cond_wait(&dg_cv[cu->cu_fd], &clnt_fd_lock);
+	while (dg_fd[cu->cu_fd].lock)
+		cond_wait(&dg_fd[cu->cu_fd].cv, &clnt_fd_lock);
 	if (__isthreaded)
                 rpc_lock_value = 1;
         else
                 rpc_lock_value = 0;
-	dg_fd_locks[cu->cu_fd] = rpc_lock_value;
+	dg_fd[cu->cu_fd].lock = rpc_lock_value;
 	mutex_unlock(&clnt_fd_lock);
 	switch (request) {
 	case CLSET_FD_CLOSE:
@@ -795,8 +786,8 @@ clnt_dg_destroy(CLIENT *cl)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (dg_fd_locks[cu_fd])
-		cond_wait(&dg_cv[cu_fd], &clnt_fd_lock);
+	while (dg_fd[cu_fd].lock)
+		cond_wait(&dg_fd[cu_fd].cv, &clnt_fd_lock);
 	if (cu->cu_closeit)
 		(void)_close(cu_fd);
 	if (cu->cu_kq >= 0)
@@ -810,7 +801,7 @@ clnt_dg_destroy(CLIENT *cl)
 	mem_free(cl, sizeof (CLIENT));
 	mutex_unlock(&clnt_fd_lock);
 	thr_sigsetmask(SIG_SETMASK, &mask, NULL);
-	cond_signal(&dg_cv[cu_fd]);
+	cond_signal(&dg_fd[cu_fd].cv);
 }
 
 static struct clnt_ops *
diff --git a/lib/libc/rpc/clnt_vc.c b/lib/libc/rpc/clnt_vc.c
index 25cd5a273531..8b0fe0dd7793 100644
--- a/lib/libc/rpc/clnt_vc.c
+++ b/lib/libc/rpc/clnt_vc.c
@@ -120,22 +120,25 @@ struct ct_data {
  *      This machinery implements per-fd locks for MT-safety.  It is not
  *      sufficient to do per-CLIENT handle locks for MT-safety because a
  *      user may create more than one CLIENT handle with the same fd behind
- *      it.  Therefore, we allocate an array of flags (vc_fd_locks), protected
- *      by the clnt_fd_lock mutex, and an array (vc_cv) of condition variables
- *      similarly protected.  Vc_fd_lock[fd] == 1 => a call is active on some
- *      CLIENT handle created for that fd.
- *      The current implementation holds locks across the entire RPC and reply.
- *      Yes, this is silly, and as soon as this code is proven to work, this
- *      should be the first thing fixed.  One step at a time.
+ *      it.  Therefore, we allocate an array of flags and condition variables
+ *      (vc_fd) protected by the clnt_fd_lock mutex.  vc_fd_lock[fd] == 1 => a
+ *      call is active on some CLIENT handle created for that fd.  The current
+ *      implementation holds locks across the entire RPC and reply.  Yes, this
+ *      is silly, and as soon as this code is proven to work, this should be
+ *      the first thing fixed.  One step at a time.
  */
-static int      *vc_fd_locks;
-static cond_t   *vc_cv;
-#define release_fd_lock(fd, mask) {	\
-	mutex_lock(&clnt_fd_lock);	\
-	vc_fd_locks[fd] = 0;		\
-	mutex_unlock(&clnt_fd_lock);	\
-	thr_sigsetmask(SIG_SETMASK, &(mask), (sigset_t *) NULL);	\
-	cond_signal(&vc_cv[fd]);	\
+static struct {
+	int lock;
+	cond_t cv;
+} *vc_fd;
+static void
+release_fd_lock(int fd, sigset_t mask)
+{
+	mutex_lock(&clnt_fd_lock);
+	vc_fd[fd].lock = 0;
+	mutex_unlock(&clnt_fd_lock);
+	thr_sigsetmask(SIG_SETMASK, &mask, (sigset_t *) NULL);
+	cond_signal(&vc_fd[fd].cv);
 }
 
 static const char clnt_vc_errstr[] = "%s : %s";
@@ -191,36 +194,23 @@ clnt_vc_create(int fd, const struct netbuf *raddr, const rpcprog_t prog,
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	if (vc_fd_locks == (int *) NULL) {
-		int cv_allocsz, fd_allocsz;
+	if (vc_fd == NULL) {
+		size_t allocsz;
+		int i;
 		int dtbsize = __rpc_dtbsize();
 
-		fd_allocsz = dtbsize * sizeof (int);
-		vc_fd_locks = (int *) mem_alloc(fd_allocsz);
-		if (vc_fd_locks == (int *) NULL) {
+		allocsz = dtbsize * sizeof (vc_fd[0]);
+		vc_fd = mem_alloc(allocsz);
+		if (vc_fd == NULL) {
 			mutex_unlock(&clnt_fd_lock);
 			thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
 			goto err;
-		} else
-			memset(vc_fd_locks, '\0', fd_allocsz);
-
-		assert(vc_cv == (cond_t *) NULL);
-		cv_allocsz = dtbsize * sizeof (cond_t);
-		vc_cv = (cond_t *) mem_alloc(cv_allocsz);
-		if (vc_cv == (cond_t *) NULL) {
-			mem_free(vc_fd_locks, fd_allocsz);
-			vc_fd_locks = (int *) NULL;
-			mutex_unlock(&clnt_fd_lock);
-			thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
-			goto err;
-		} else {
-			int i;
-
-			for (i = 0; i < dtbsize; i++)
-				cond_init(&vc_cv[i], 0, (void *) 0);
 		}
-	} else
-		assert(vc_cv != (cond_t *) NULL);
+		memset(vc_fd, '\0', allocsz);
+
+		for (i = 0; i < dtbsize; i++)
+			cond_init(&vc_fd[i].cv, 0, (void *) 0);
+	}
 
 	/*
 	 * XXX - fvdl connecting while holding a mutex?
@@ -331,13 +321,13 @@ clnt_vc_call(CLIENT *cl, rpcproc_t proc, xdrproc_t xdr_args, void *args_ptr,
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (vc_fd_locks[ct->ct_fd])
-		cond_wait(&vc_cv[ct->ct_fd], &clnt_fd_lock);
+	while (vc_fd[ct->ct_fd].lock)
+		cond_wait(&vc_fd[ct->ct_fd].cv, &clnt_fd_lock);
 	if (__isthreaded)
                 rpc_lock_value = 1;
         else
                 rpc_lock_value = 0;
-	vc_fd_locks[ct->ct_fd] = rpc_lock_value;
+	vc_fd[ct->ct_fd].lock = rpc_lock_value;
 	mutex_unlock(&clnt_fd_lock);
 	if (!ct->ct_waitset) {
 		/* If time is not within limits, we ignore it. */
@@ -484,13 +474,13 @@ clnt_vc_freeres(CLIENT *cl, xdrproc_t xdr_res, void *res_ptr)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (vc_fd_locks[ct->ct_fd])
-		cond_wait(&vc_cv[ct->ct_fd], &clnt_fd_lock);
+	while (vc_fd[ct->ct_fd].lock)
+		cond_wait(&vc_fd[ct->ct_fd].cv, &clnt_fd_lock);
 	xdrs->x_op = XDR_FREE;
 	dummy = (*xdr_res)(xdrs, res_ptr);
 	mutex_unlock(&clnt_fd_lock);
 	thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
-	cond_signal(&vc_cv[ct->ct_fd]);
+	cond_signal(&vc_fd[ct->ct_fd].cv);
 
 	return dummy;
 }
@@ -531,13 +521,13 @@ clnt_vc_control(CLIENT *cl, u_int request, void *info)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (vc_fd_locks[ct->ct_fd])
-		cond_wait(&vc_cv[ct->ct_fd], &clnt_fd_lock);
+	while (vc_fd[ct->ct_fd].lock)
+		cond_wait(&vc_fd[ct->ct_fd].cv, &clnt_fd_lock);
 	if (__isthreaded)
                 rpc_lock_value = 1;
         else
                 rpc_lock_value = 0;
-	vc_fd_locks[ct->ct_fd] = rpc_lock_value;
+	vc_fd[ct->ct_fd].lock = rpc_lock_value;
 	mutex_unlock(&clnt_fd_lock);
 
 	switch (request) {
@@ -648,8 +638,8 @@ clnt_vc_destroy(CLIENT *cl)
 	sigfillset(&newmask);
 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
 	mutex_lock(&clnt_fd_lock);
-	while (vc_fd_locks[ct_fd])
-		cond_wait(&vc_cv[ct_fd], &clnt_fd_lock);
+	while (vc_fd[ct_fd].lock)
+		cond_wait(&vc_fd[ct_fd].cv, &clnt_fd_lock);
 	if (ct->ct_closeit && ct->ct_fd != -1) {
 		(void)_close(ct->ct_fd);
 	}
@@ -663,7 +653,7 @@ clnt_vc_destroy(CLIENT *cl)
 	mem_free(cl, sizeof(CLIENT));
 	mutex_unlock(&clnt_fd_lock);
 	thr_sigsetmask(SIG_SETMASK, &(mask), NULL);
-	cond_signal(&vc_cv[ct_fd]);
+	cond_signal(&vc_fd[ct_fd].cv);
 }
 
 /*