maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Monty,

Thanks for you reply. Please allow me to disagree with some points. :)

Attached please find 2 patches, both fix this memory leak. One approach is
for detached threads, another one is for joinable threads.

On Thu, May 05, 2016 at 02:12:17PM +0300, Michael Widenius wrote:
> Hi!
> 
> On Wed, May 4, 2016 at 11:13 AM, Sergey Vojtovich <svoj@xxxxxxxxxxx> wrote:
> > Hi Monty,
> >
> > I vaguely recall you told me once that you like detached threads more than
> > joinable, specifically for service threads. Could you remind me what were
> > the reasons behind it?
> >
> > I need this to fix https://jira.mariadb.org/browse/MDEV-9994 properly.
> 
> The main reason is just that with joinable threads you have more work
> to manage the threads as you need to remember to join them as
> otherwise you will get a memory loss.
I believe joinable.patch prooves the opposite, that is joinable threads are
easier to manage.

With joinable.patch we have 32 lines less, while with detached.patch we have 4
lines more.

> 
> The only time the server has a benefit of join able threads, at least
> as far as I am aware of it, is at shutdown, which hasn't been a real
> problem as in most cases
> my_thread_end() will ensure that all threads has been calling
> my_thread_end() before the server dies.
How can my_thread_end() ensure my_thread_end() is called before server dies?

I believe detached threads are only beneficial if we don't care about clean
thread shutdown (that is main thread may exit any time independenly of other
threads state). Otherwise we have to add code synchronizing thread shutdown
like for checkpoint thread.

> 
> Referring to the valgrind issue, I would just ignore the memory loss
> for tls, as this isn't a real error.  We are already ignoring this on
> other platforms.
I agree it's minor. I would agree to suppress the warning if it was generated
by third party library and we could do nothing about it. But this memory leak
is a consequence of API violation on MariaDB side.

I'd suggest to remove all these suppressions. Even if we get warnings from
other threads - it's trivial to fix.

Thanks,
Sergey

diff --git a/storage/maria/ma_checkpoint.c b/storage/maria/ma_checkpoint.c
index de8a9610..2bbbfb0 100644
--- a/storage/maria/ma_checkpoint.c
+++ b/storage/maria/ma_checkpoint.c
@@ -46,7 +46,7 @@ static mysql_mutex_t LOCK_checkpoint;
 static mysql_cond_t  COND_checkpoint;
 /** @brief control structure for checkpoint background thread */
 static MA_SERVICE_THREAD_CONTROL checkpoint_control=
-  {THREAD_DEAD, FALSE, &LOCK_checkpoint, &COND_checkpoint};
+  {0, FALSE, FALSE, &LOCK_checkpoint, &COND_checkpoint};
 /* is ulong like pagecache->blocks_changed */
 static ulong pages_to_flush_before_next_checkpoint;
 static PAGECACHE_FILE *dfiles, /**< data files to flush in background */
@@ -326,7 +326,6 @@ static int really_execute_checkpoint(void)
 
 int ma_checkpoint_init(ulong interval)
 {
-  pthread_t th;
   int res= 0;
   DBUG_ENTER("ma_checkpoint_init");
   if (ma_service_thread_control_init(&checkpoint_control))
@@ -335,11 +334,12 @@ int ma_checkpoint_init(ulong interval)
   {
     compile_time_assert(sizeof(void *) >= sizeof(ulong));
     if (!(res= mysql_thread_create(key_thread_checkpoint,
-                                   &th, NULL, ma_checkpoint_background,
+                                   &checkpoint_control.thread, NULL,
+                                   ma_checkpoint_background,
                                    (void *)interval)))
     {
       /* thread lives, will have to be killed */
-      checkpoint_control.status= THREAD_RUNNING;
+      checkpoint_control.alive= TRUE;
     }
   }
   DBUG_RETURN(res);
@@ -721,7 +721,6 @@ pthread_handler_t ma_checkpoint_background(void *arg)
     DBUG_EXECUTE_IF("maria_checkpoint_indirect", level= CHECKPOINT_INDIRECT;);
     ma_checkpoint_execute(level, FALSE);
   }
-  my_service_thread_signal_end(&checkpoint_control);
   my_thread_end();
   return 0;
 }
diff --git a/storage/maria/ma_loghandler.c b/storage/maria/ma_loghandler.c
index 2538dee..295487a 100644
--- a/storage/maria/ma_loghandler.c
+++ b/storage/maria/ma_loghandler.c
@@ -54,7 +54,7 @@ static mysql_mutex_t LOCK_soft_sync;
 static mysql_cond_t  COND_soft_sync;
 /** @brief control structure for checkpoint background thread */
 static MA_SERVICE_THREAD_CONTROL soft_sync_control=
-  {THREAD_DEAD, FALSE, &LOCK_soft_sync, &COND_soft_sync};
+  {0, FALSE, FALSE, &LOCK_soft_sync, &COND_soft_sync};
 
 
 /* transaction log file descriptor */
@@ -8819,7 +8819,6 @@ ma_soft_sync_background( void *arg __attribute__((unused)))
       if (my_service_thread_sleep(&soft_sync_control, sleep))
         break;
     }
-    my_service_thread_signal_end(&soft_sync_control);
     my_thread_end();
     DBUG_RETURN(0);
   }
@@ -8832,7 +8831,6 @@ ma_soft_sync_background( void *arg __attribute__((unused)))
 
 int translog_soft_sync_start(void)
 {
-  pthread_t th;
   int res= 0;
   uint32 min, max;
   DBUG_ENTER("translog_soft_sync_start");
@@ -8848,8 +8846,9 @@ int translog_soft_sync_start(void)
 
   if (!(res= ma_service_thread_control_init(&soft_sync_control)))
     if (!(res= mysql_thread_create(key_thread_soft_sync,
-                                   &th, NULL, ma_soft_sync_background, NULL)))
-      soft_sync_control.status= THREAD_RUNNING;
+                                   &soft_sync_control.thread, NULL,
+                                   ma_soft_sync_background, NULL)))
+      soft_sync_control.alive= TRUE;
   DBUG_RETURN(res);
 }
 
diff --git a/storage/maria/ma_servicethread.c b/storage/maria/ma_servicethread.c
index e5c949a..8ecb2d6 100644
--- a/storage/maria/ma_servicethread.c
+++ b/storage/maria/ma_servicethread.c
@@ -33,7 +33,7 @@ int ma_service_thread_control_init(MA_SERVICE_THREAD_CONTROL *control)
   DBUG_ENTER("ma_service_thread_control_init");
   DBUG_PRINT("init", ("control 0x%lx", (ulong) control));
   control->inited= TRUE;
-  control->status= THREAD_DEAD; /* not yet born == dead */
+  control->alive= FALSE;
   res= (mysql_mutex_init(key_SERVICE_THREAD_CONTROL_lock,
                          control->LOCK_control, MY_MUTEX_INIT_SLOW) ||
         mysql_cond_init(key_SERVICE_THREAD_CONTROL_cond,
@@ -60,20 +60,17 @@ void ma_service_thread_control_end(MA_SERVICE_THREAD_CONTROL *control)
   DBUG_PRINT("init", ("control 0x%lx", (ulong) control));
   DBUG_ASSERT(control->inited);
   mysql_mutex_lock(control->LOCK_control);
-  if (control->status != THREAD_DEAD) /* thread was started OK */
+  if (control->alive)
   {
     DBUG_PRINT("info",("killing Maria background thread"));
-    control->status= THREAD_DYING; /* kill it */
-    do /* and wait for it to be dead */
-    {
-      /* wake it up if it was in a sleep */
-      mysql_cond_broadcast(control->COND_control);
-      DBUG_PRINT("info",("waiting for Maria background thread to die"));
-      mysql_cond_wait(control->COND_control, control->LOCK_control);
-    }
-    while (control->status != THREAD_DEAD);
+    control->alive= FALSE; /* kill it */
+    mysql_cond_broadcast(control->COND_control);
+    mysql_mutex_unlock(control->LOCK_control);
+    DBUG_PRINT("info", ("waiting for Maria background thread to die"));
+    pthread_join(control->thread, NULL);
   }
-  mysql_mutex_unlock(control->LOCK_control);
+  else
+    mysql_mutex_unlock(control->LOCK_control);
   mysql_mutex_destroy(control->LOCK_control);
   mysql_cond_destroy(control->COND_control);
   control->inited= FALSE;
@@ -100,7 +97,7 @@ my_bool my_service_thread_sleep(MA_SERVICE_THREAD_CONTROL *control,
   DBUG_ENTER("my_service_thread_sleep");
   DBUG_PRINT("init", ("control 0x%lx", (ulong) control));
   mysql_mutex_lock(control->LOCK_control);
-  if (control->status == THREAD_DYING)
+  if (!control->alive)
   {
     mysql_mutex_unlock(control->LOCK_control);
     DBUG_RETURN(TRUE);
@@ -119,34 +116,8 @@ my_bool my_service_thread_sleep(MA_SERVICE_THREAD_CONTROL *control,
                            control->LOCK_control, &abstime);
   }
 #endif
-  if (control->status == THREAD_DYING)
+  if (!control->alive)
     res= TRUE;
   mysql_mutex_unlock(control->LOCK_control);
   DBUG_RETURN(res);
 }
-
-
-/**
-  inform about thread exiting
-
-  @param control        control block
-*/
-
-void my_service_thread_signal_end(MA_SERVICE_THREAD_CONTROL *control)
-{
-  DBUG_ENTER("my_service_thread_signal_end");
-  DBUG_PRINT("init", ("control 0x%lx", (ulong) control));
-  mysql_mutex_lock(control->LOCK_control);
-  control->status = THREAD_DEAD; /* indicate that we are dead */
-  /*
-    wake up ma_service_thread_control_end which may be waiting for
-    our death
-  */
-  mysql_cond_broadcast(control->COND_control);
-  /*
-    broadcast was inside unlock because ma_service_thread_control_end
-    destroys mutex
-  */
-  mysql_mutex_unlock(control->LOCK_control);
-  DBUG_VOID_RETURN;
-}
diff --git a/storage/maria/ma_servicethread.h b/storage/maria/ma_servicethread.h
index ed578d9..e62cf91 100644
--- a/storage/maria/ma_servicethread.h
+++ b/storage/maria/ma_servicethread.h
@@ -16,12 +16,11 @@
 
 #include <my_pthread.h>
 
-enum ma_service_thread_state {THREAD_RUNNING, THREAD_DYING, THREAD_DEAD};
-
 typedef struct st_ma_service_thread_control
 {
-  /** 'kill' flag for the background thread */
-  enum ma_service_thread_state status;
+  pthread_t thread;
+  /** 'alive' flag for the background thread */
+  my_bool alive;
   /** if thread module was inited or not */
   my_bool inited;
   /** for killing the background thread */

diff --git a/storage/maria/ma_checkpoint.c b/storage/maria/ma_checkpoint.c
index de8a9610..6f2a2ff 100644
--- a/storage/maria/ma_checkpoint.c
+++ b/storage/maria/ma_checkpoint.c
@@ -333,14 +333,18 @@ int ma_checkpoint_init(ulong interval)
     res= 1;
   else if (interval > 0)
   {
+    pthread_attr_t attr;
     compile_time_assert(sizeof(void *) >= sizeof(ulong));
+    pthread_attr_init(&attr);
+    pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
     if (!(res= mysql_thread_create(key_thread_checkpoint,
-                                   &th, NULL, ma_checkpoint_background,
+                                   &th, &attr, ma_checkpoint_background,
                                    (void *)interval)))
     {
       /* thread lives, will have to be killed */
       checkpoint_control.status= THREAD_RUNNING;
     }
+    pthread_attr_destroy(&attr);
   }
   DBUG_RETURN(res);
 }