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
Signed-off-by: Doug Nazar <[email protected]>
---
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
Signed-off-by: Doug Nazar <[email protected]>
---
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
Signed-off-by: Doug Nazar <[email protected]>
---
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
Signed-off-by: Doug Nazar <[email protected]>
---
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
On 7/22/20 1:34 AM, Doug Nazar wrote:
> Signed-off-by: Doug Nazar <[email protected]>
> ---
> 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.
On 7/22/20 1:34 AM, Doug Nazar wrote:
> Signed-off-by: Doug Nazar <[email protected]>
> ---
> 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.