Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] libtirpc patches
@ 2020-07-22  5:34 Doug Nazar
  2020-07-22  5:34 ` [PATCH 1/5] svc_dg: Free xp_netid during destroy Doug Nazar
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Doug Nazar @ 2020-07-22  5:34 UTC (permalink / raw)
  To: libtirpc-devel; +Cc: linux-nfs

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] svc_dg: Free xp_netid during destroy
  2020-07-22  5:34 [PATCH 0/5] libtirpc patches Doug Nazar
@ 2020-07-22  5:34 ` Doug Nazar
  2020-07-22  5:34 ` [PATCH 2/5] svc: Batch allocations of pollfds Doug Nazar
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Doug Nazar @ 2020-07-22  5:34 UTC (permalink / raw)
  To: libtirpc-devel; +Cc: linux-nfs

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/5] svc: Batch allocations of pollfds
  2020-07-22  5:34 [PATCH 0/5] libtirpc patches Doug Nazar
  2020-07-22  5:34 ` [PATCH 1/5] svc_dg: Free xp_netid during destroy Doug Nazar
@ 2020-07-22  5:34 ` Doug Nazar
  2020-07-29 14:20   ` Steve Dickson
  2020-07-22  5:34 ` [PATCH 3/5] Add destructor functions to cleanup static resources on exit Doug Nazar
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Doug Nazar @ 2020-07-22  5:34 UTC (permalink / raw)
  To: libtirpc-devel; +Cc: linux-nfs

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/5] Add destructor functions to cleanup static resources on exit
  2020-07-22  5:34 [PATCH 0/5] libtirpc patches Doug Nazar
  2020-07-22  5:34 ` [PATCH 1/5] svc_dg: Free xp_netid during destroy Doug Nazar
  2020-07-22  5:34 ` [PATCH 2/5] svc: Batch allocations of pollfds Doug Nazar
@ 2020-07-22  5:34 ` Doug Nazar
  2020-07-22  5:34 ` [PATCH 4/5] Add ability to detect if we're on the main thread Doug Nazar
  2020-07-22  5:34 ` [PATCH 5/5] Use static object on main thread, instead of thread specific data Doug Nazar
  4 siblings, 0 replies; 8+ messages in thread
From: Doug Nazar @ 2020-07-22  5:34 UTC (permalink / raw)
  To: libtirpc-devel; +Cc: linux-nfs

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4/5] Add ability to detect if we're on the main thread.
  2020-07-22  5:34 [PATCH 0/5] libtirpc patches Doug Nazar
                   ` (2 preceding siblings ...)
  2020-07-22  5:34 ` [PATCH 3/5] Add destructor functions to cleanup static resources on exit Doug Nazar
@ 2020-07-22  5:34 ` Doug Nazar
  2020-07-29 14:27   ` Steve Dickson
  2020-07-22  5:34 ` [PATCH 5/5] Use static object on main thread, instead of thread specific data Doug Nazar
  4 siblings, 1 reply; 8+ messages in thread
From: Doug Nazar @ 2020-07-22  5:34 UTC (permalink / raw)
  To: libtirpc-devel; +Cc: linux-nfs

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 5/5] Use static object on main thread, instead of thread specific data.
  2020-07-22  5:34 [PATCH 0/5] libtirpc patches Doug Nazar
                   ` (3 preceding siblings ...)
  2020-07-22  5:34 ` [PATCH 4/5] Add ability to detect if we're on the main thread Doug Nazar
@ 2020-07-22  5:34 ` Doug Nazar
  4 siblings, 0 replies; 8+ messages in thread
From: Doug Nazar @ 2020-07-22  5:34 UTC (permalink / raw)
  To: libtirpc-devel; +Cc: linux-nfs

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/5] svc: Batch allocations of pollfds
  2020-07-22  5:34 ` [PATCH 2/5] svc: Batch allocations of pollfds Doug Nazar
@ 2020-07-29 14:20   ` Steve Dickson
  0 siblings, 0 replies; 8+ messages in thread
From: Steve Dickson @ 2020-07-29 14:20 UTC (permalink / raw)
  To: Doug Nazar, libtirpc-devel; +Cc: linux-nfs



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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 4/5] Add ability to detect if we're on the main thread.
  2020-07-22  5:34 ` [PATCH 4/5] Add ability to detect if we're on the main thread Doug Nazar
@ 2020-07-29 14:27   ` Steve Dickson
  0 siblings, 0 replies; 8+ messages in thread
From: Steve Dickson @ 2020-07-29 14:27 UTC (permalink / raw)
  To: Doug Nazar, libtirpc-devel; +Cc: linux-nfs



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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22  5:34 [PATCH 0/5] libtirpc patches Doug Nazar
2020-07-22  5:34 ` [PATCH 1/5] svc_dg: Free xp_netid during destroy Doug Nazar
2020-07-22  5:34 ` [PATCH 2/5] svc: Batch allocations of pollfds Doug Nazar
2020-07-29 14:20   ` Steve Dickson
2020-07-22  5:34 ` [PATCH 3/5] Add destructor functions to cleanup static resources on exit Doug Nazar
2020-07-22  5:34 ` [PATCH 4/5] Add ability to detect if we're on the main thread Doug Nazar
2020-07-29 14:27   ` Steve Dickson
2020-07-22  5:34 ` [PATCH 5/5] Use static object on main thread, instead of thread specific data Doug Nazar

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git