[PATCH 0/5] libtirpc patches

[PATCH 0/5] libtirpc patches

[Doug Nazar]

While running valgrind on various nfs-utils programs, I came across a leak
while destroying a svc_dg object.

Second, I noticed that pollfds were being reallocated a dozen times, so
added an optimization to batch the allocations.

Third, I added destructor functions to clear up some of the static allocations
on exit. This is mainly to help with running valgrind. Not too important, more
of an RFC if it's desired.

Forth, I noticed that the comments expected the main thread to use the static
variable, instead of thread specific data but wasn't doing that. The last two
patches implement that. Also helps with valgrind as TSD is not destroyed on
the main thread.

Thanks,
Doug

Doug Nazar (5):
  svc_dg: Free xp_netid during destroy
  svc: Batch allocations of pollfds
  Add destructor functions to cleanup static resources on exit
  Add ability to detect if we're on the main thread.
  Use static object on main thread, instead of thread specific data.

 src/auth_none.c    | 14 ++++++++++++++
 src/clnt_dg.c      | 12 ++++++++++++
 src/clnt_vc.c      | 12 ++++++++++++
 src/getnetconfig.c |  3 +++
 src/mt_misc.c      | 20 ++++++++++++++++++++
 src/svc.c          | 35 ++++++++++++++++++++++++++++-------
 src/svc_dg.c       |  2 ++
 tirpc/reentrant.h  |  1 +
 8 files changed, 92 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/5] svc_dg: Free xp_netid during destroy

[Doug Nazar]

Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 src/svc_dg.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/svc_dg.c b/src/svc_dg.c
index 894ee9b..a9f63ff 100644
--- a/src/svc_dg.c
+++ b/src/svc_dg.c
@@ -328,6 +328,8 @@ svc_dg_destroy(xprt)
 		(void) mem_free(xprt->xp_ltaddr.buf, xprt->xp_ltaddr.maxlen);
 	if (xprt->xp_tp)
 		(void) free(xprt->xp_tp);
+	if (xprt->xp_netid)
+		(void) free(xprt->xp_netid);
 	(void) mem_free(xprt, sizeof (SVCXPRT));
 }
 
-- 
2.26.2

[PATCH 2/5] svc: Batch allocations of pollfds

[Doug Nazar]

Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 src/svc.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/src/svc.c b/src/svc.c
index 6db164b..57f7ba3 100644
--- a/src/svc.c
+++ b/src/svc.c
@@ -54,6 +54,7 @@
 #include "rpc_com.h"
 
 #define	RQCRED_SIZE	400	/* this size is excessive */
+#define	SVC_POLLFD_INCREMENT	16
 
 #define max(a, b) (a > b ? a : b)
 
@@ -107,6 +108,7 @@ xprt_register (xprt)
   if (sock < _rpc_dtablesize())
     {
       int i;
+      size_t size;
       struct pollfd *new_svc_pollfd;
 
       __svc_xports[sock] = xprt;
@@ -126,17 +128,17 @@ xprt_register (xprt)
             goto unlock;
           }
 
-      new_svc_pollfd = (struct pollfd *) realloc (svc_pollfd,
-                                                  sizeof (struct pollfd)
-                                                  * (svc_max_pollfd + 1));
+      size = sizeof (struct pollfd) * (svc_max_pollfd + SVC_POLLFD_INCREMENT);
+      new_svc_pollfd = (struct pollfd *) realloc (svc_pollfd, size);
       if (new_svc_pollfd == NULL) /* Out of memory */
         goto unlock;
       svc_pollfd = new_svc_pollfd;
-      ++svc_max_pollfd;
+      svc_max_pollfd += SVC_POLLFD_INCREMENT;
 
-      svc_pollfd[svc_max_pollfd - 1].fd = sock;
-      svc_pollfd[svc_max_pollfd - 1].events = (POLLIN | POLLPRI |
-                                               POLLRDNORM | POLLRDBAND);
+      svc_pollfd[i].fd = sock;
+      svc_pollfd[i].events = (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND);
+      for (++i; i < svc_max_pollfd; ++i)
+        svc_pollfd[i].fd = -1;
     }
 unlock:
   rwlock_unlock (&svc_fd_lock);
-- 
2.26.2

Re: [PATCH 2/5] svc: Batch allocations of pollfds

[Steve Dickson]

On 7/22/20 1:34 AM, Doug Nazar wrote:
> Signed-off-by: Doug Nazar <nazard@nazar.ca>
> ---
>  src/svc.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/svc.c b/src/svc.c
> index 6db164b..57f7ba3 100644
> --- a/src/svc.c
> +++ b/src/svc.c
> @@ -54,6 +54,7 @@
>  #include "rpc_com.h"
>  
>  #define	RQCRED_SIZE	400	/* this size is excessive */
> +#define	SVC_POLLFD_INCREMENT	16
>  
>  #define max(a, b) (a > b ? a : b)
>  
> @@ -107,6 +108,7 @@ xprt_register (xprt)
>    if (sock < _rpc_dtablesize())
>      {
>        int i;
> +      size_t size;
>        struct pollfd *new_svc_pollfd;
>  
>        __svc_xports[sock] = xprt;
> @@ -126,17 +128,17 @@ xprt_register (xprt)
>              goto unlock;
>            }
>  
> -      new_svc_pollfd = (struct pollfd *) realloc (svc_pollfd,
> -                                                  sizeof (struct pollfd)
> -                                                  * (svc_max_pollfd + 1));
> +      size = sizeof (struct pollfd) * (svc_max_pollfd + SVC_POLLFD_INCREMENT);
> +      new_svc_pollfd = (struct pollfd *) realloc (svc_pollfd, size);
>        if (new_svc_pollfd == NULL) /* Out of memory */
>          goto unlock;
>        svc_pollfd = new_svc_pollfd;
> -      ++svc_max_pollfd;
> +      svc_max_pollfd += SVC_POLLFD_INCREMENT;
>  
> -      svc_pollfd[svc_max_pollfd - 1].fd = sock;
> -      svc_pollfd[svc_max_pollfd - 1].events = (POLLIN | POLLPRI |
> -                                               POLLRDNORM | POLLRDBAND);
> +      svc_pollfd[i].fd = sock;
> +      svc_pollfd[i].events = (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND);
> +      for (++i; i < svc_max_pollfd; ++i)
> +        svc_pollfd[i].fd = -1;
>      }
>  unlock:
>    rwlock_unlock (&svc_fd_lock);
> 
Just curious as why batch allocations are need? What problem does it solve?

steved.

[PATCH 3/5] Add destructor functions to cleanup static resources on exit

[Doug Nazar]

Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 src/auth_none.c | 14 ++++++++++++++
 src/clnt_dg.c   | 12 ++++++++++++
 src/clnt_vc.c   | 12 ++++++++++++
 src/svc.c       | 19 +++++++++++++++++++
 4 files changed, 57 insertions(+)

diff --git a/src/auth_none.c b/src/auth_none.c
index 0b0bbd1..06db3b3 100644
--- a/src/auth_none.c
+++ b/src/auth_none.c
@@ -72,6 +72,20 @@ static struct authnone_private {
 	u_int	mcnt;
 } *authnone_private;
 
+__attribute__((destructor))
+static void
+authnone_cleanup(void)
+{
+	extern mutex_t authnone_lock;
+
+	mutex_lock(&authnone_lock);
+	if (authnone_private) {
+		free(authnone_private);
+		authnone_private = NULL;
+	}
+	mutex_unlock(&authnone_lock);
+}
+
 AUTH *
 authnone_create()
 {
diff --git a/src/clnt_dg.c b/src/clnt_dg.c
index abc09f1..60e3503 100644
--- a/src/clnt_dg.c
+++ b/src/clnt_dg.c
@@ -130,6 +130,18 @@ struct cu_data {
 	char			cu_inbuf[1];
 };
 
+__attribute__((destructor))
+static void
+clnt_vc_cleanup(void)
+{
+	mutex_lock(&clnt_fd_lock);
+	if (dg_fd_locks) {
+		mem_free(dg_fd_locks, sizeof(*dg_fd_locks));
+		dg_fd_locks = NULL;
+	}
+	mutex_unlock(&clnt_fd_lock);
+}
+
 /*
  * Connection less client creation returns with client handle parameters.
  * Default options are set, which the user can change using clnt_control().
diff --git a/src/clnt_vc.c b/src/clnt_vc.c
index 6f7f7da..a4b41df 100644
--- a/src/clnt_vc.c
+++ b/src/clnt_vc.c
@@ -158,6 +158,18 @@ static const char clnt_vc_errstr[] = "%s : %s";
 static const char clnt_vc_str[] = "clnt_vc_create";
 static const char __no_mem_str[] = "out of memory";
 
+__attribute__((destructor))
+static void
+clnt_vc_cleanup(void)
+{
+	mutex_lock(&clnt_fd_lock);
+	if (vc_fd_locks) {
+		mem_free(vc_fd_locks, sizeof(*vc_fd_locks));
+		vc_fd_locks = NULL;
+	}
+	mutex_unlock(&clnt_fd_lock);
+}
+
 /*
  * Create a client handle for a connection.
  * Default options are set, which the user can change using clnt_control()'s.
diff --git a/src/svc.c b/src/svc.c
index 57f7ba3..b5f35c8 100644
--- a/src/svc.c
+++ b/src/svc.c
@@ -85,6 +85,25 @@ static void __xprt_do_unregister (SVCXPRT * xprt, bool_t dolock);
 
 /* ***************  SVCXPRT related stuff **************** */
 
+__attribute__((destructor))
+static void
+xprt_cleanup(void)
+{
+  rwlock_wrlock (&svc_fd_lock);
+  if (__svc_xports != NULL)
+    {
+      free (__svc_xports);
+      __svc_xports = NULL;
+    }
+  if (svc_pollfd != NULL)
+    {
+      free (svc_pollfd);
+      svc_pollfd = NULL;
+      svc_max_pollfd = 0;
+    }
+  rwlock_unlock (&svc_fd_lock);
+}
+
 /*
  * Activate a transport handle.
  */
-- 
2.26.2

[PATCH 4/5] Add ability to detect if we're on the main thread.

[Doug Nazar]

Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 src/mt_misc.c     | 17 +++++++++++++++++
 tirpc/reentrant.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/src/mt_misc.c b/src/mt_misc.c
index 5a49b78..020b55d 100644
--- a/src/mt_misc.c
+++ b/src/mt_misc.c
@@ -151,3 +151,20 @@ void tsd_key_delete(void)
 	return;
 }
 
+static pthread_t main_thread_id;
+
+__attribute__((constructor))
+static void
+get_thread_id(void)
+{
+	/* This will only work if we're opened by the main thread.
+	 * Shouldn't be a problem in practice since we expect to be
+	 * linked against, not dlopen() from a random thread.
+	 */
+	main_thread_id = pthread_self();
+}
+
+int thr_main(void)
+{
+	return pthread_equal(main_thread_id, pthread_self());
+}
diff --git a/tirpc/reentrant.h b/tirpc/reentrant.h
index 5bb581a..ee65454 100644
--- a/tirpc/reentrant.h
+++ b/tirpc/reentrant.h
@@ -76,4 +76,5 @@
 #define thr_self()		pthread_self()
 #define thr_exit(x)		pthread_exit(x)
 
+extern int thr_main(void);
 #endif
-- 
2.26.2

Re: [PATCH 4/5] Add ability to detect if we're on the main thread.

[Steve Dickson]

On 7/22/20 1:34 AM, Doug Nazar wrote:
> Signed-off-by: Doug Nazar <nazard@nazar.ca>
> ---
>  src/mt_misc.c     | 17 +++++++++++++++++
>  tirpc/reentrant.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/src/mt_misc.c b/src/mt_misc.c
> index 5a49b78..020b55d 100644
> --- a/src/mt_misc.c
> +++ b/src/mt_misc.c
> @@ -151,3 +151,20 @@ void tsd_key_delete(void)
>  	return;
>  }
>  
> +static pthread_t main_thread_id;
> +
> +__attribute__((constructor))
> +static void
> +get_thread_id(void)
> +{
> +	/* This will only work if we're opened by the main thread.
> +	 * Shouldn't be a problem in practice since we expect to be
> +	 * linked against, not dlopen() from a random thread.
> +	 */
> +	main_thread_id = pthread_self();
> +}
> +
> +int thr_main(void)
> +{
> +	return pthread_equal(main_thread_id, pthread_self());
> +}
> diff --git a/tirpc/reentrant.h b/tirpc/reentrant.h
> index 5bb581a..ee65454 100644
> --- a/tirpc/reentrant.h
> +++ b/tirpc/reentrant.h
> @@ -76,4 +76,5 @@
>  #define thr_self()		pthread_self()
>  #define thr_exit(x)		pthread_exit(x)
>  
> +extern int thr_main(void);
>  #endif
> 
Again... why is this needed? 

Your description part of these patches are a bit thin ;-)

steved.

[PATCH 5/5] Use static object on main thread, instead of thread specific data.

[Doug Nazar]

Signed-off-by: Doug Nazar <nazard@nazar.ca>
---
 src/getnetconfig.c | 3 +++
 src/mt_misc.c      | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/getnetconfig.c b/src/getnetconfig.c
index cfd33c2..3a27367 100644
--- a/src/getnetconfig.c
+++ b/src/getnetconfig.c
@@ -136,6 +136,9 @@ __nc_error()
 	 * (including non-threaded programs), or if an allocation
 	 * fails.
 	 */
+	if (thr_main())
+		return (&nc_error);
+
 	if (nc_key == KEY_INITIALIZER) {
 		error = 0;
 		mutex_lock(&nc_lock);
diff --git a/src/mt_misc.c b/src/mt_misc.c
index 020b55d..79deaa3 100644
--- a/src/mt_misc.c
+++ b/src/mt_misc.c
@@ -112,6 +112,9 @@ __rpc_createerr()
 {
 	struct rpc_createerr *rce_addr;
 
+	if (thr_main())
+		return (&rpc_createerr);
+
 	mutex_lock(&tsd_lock);
 	if (rce_key == KEY_INITIALIZER)
 		thr_keycreate(&rce_key, free);
-- 
2.26.2