Skip to content

Commit

Permalink
Move all kill mutex protection to LOCK_thd_kill
Browse files Browse the repository at this point in the history
LOCK_thd_data was used to protect both THD data and
ensure that the THD is not deleted while it was in use

This patch moves the THD delete protection to LOCK_thd_kill,
which already protects the THD for kill.

The benefits are:
- More well defined what LOCK_thd_data protects
- LOCK_thd_data usage is now much simpler and easier to verify
- Less chance of deadlocks in SHOW PROCESS LIST as there is less
  chance of interactions between mutexes
- Remove not needed LOCK_thread_count from
  thd_get_error_context_description()
- Fewer mutex taken for thd->awake()

Other things:
- Don't take mysys->var mutex in show processlist to check if thread
  is kill marked
- thd->awake() now automatically takes the LOCK_thd_kill mutex
  (Simplifies code)
- Apc uses LOCK_thd_kill instead of LOCK_thd_data
  • Loading branch information
montywi committed Dec 8, 2017
1 parent 3574f9c commit c2118a0
Show file tree
Hide file tree
Showing 15 changed files with 109 additions and 120 deletions.
14 changes: 7 additions & 7 deletions mysql-test/suite/perfschema/r/nesting.result
Expand Up @@ -125,7 +125,7 @@ relative_event_id relative_end_event_id event_name comment nesting_event_type re
15 15 stage/sql/Starting cleanup (stage) STATEMENT 0
16 16 stage/sql/Freeing items (stage) STATEMENT 0
17 17 wait/io/socket/sql/client_connection send STATEMENT 0
18 18 wait/synch/mutex/sql/THD::LOCK_thd_data lock STATEMENT 0
18 18 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STATEMENT 0
19 20 stage/sql/Reset for next command (stage) STATEMENT 0
20 20 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 19
21 21 idle idle NULL NULL
Expand All @@ -147,7 +147,7 @@ relative_event_id relative_end_event_id event_name comment nesting_event_type re
37 37 stage/sql/Starting cleanup (stage) STATEMENT 22
38 38 stage/sql/Freeing items (stage) STATEMENT 22
39 39 wait/io/socket/sql/client_connection send STATEMENT 22
40 40 wait/synch/mutex/sql/THD::LOCK_thd_data lock STATEMENT 22
40 40 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STATEMENT 22
41 42 stage/sql/Reset for next command (stage) STATEMENT 22
42 42 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 41
43 43 idle idle NULL NULL
Expand All @@ -169,7 +169,7 @@ relative_event_id relative_end_event_id event_name comment nesting_event_type re
59 59 stage/sql/Starting cleanup (stage) STATEMENT 44
60 60 stage/sql/Freeing items (stage) STATEMENT 44
61 61 wait/io/socket/sql/client_connection send STATEMENT 44
62 62 wait/synch/mutex/sql/THD::LOCK_thd_data lock STATEMENT 44
62 62 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STATEMENT 44
63 64 stage/sql/Reset for next command (stage) STATEMENT 44
64 64 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 63
65 65 idle idle NULL NULL
Expand All @@ -194,7 +194,7 @@ select "With a third part to make things complete" as payload NULL NULL
82 82 stage/sql/Starting cleanup (stage) STATEMENT 66
83 85 stage/sql/Freeing items (stage) STATEMENT 66
84 84 wait/io/socket/sql/client_connection send STAGE 83
85 85 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 83
85 85 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STAGE 83
86 103 statement/sql/select select "And this is the second part of a multi query" as payload;
select "With a third part to make things complete" as payload NULL NULL
87 89 stage/sql/Init (stage) STATEMENT 86
Expand All @@ -213,7 +213,7 @@ select "With a third part to make things complete" as payload NULL NULL
100 100 stage/sql/Starting cleanup (stage) STATEMENT 86
101 103 stage/sql/Freeing items (stage) STATEMENT 86
102 102 wait/io/socket/sql/client_connection send STAGE 101
103 103 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 101
103 103 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STAGE 101
104 122 statement/sql/select select "With a third part to make things complete" as payload NULL NULL
105 106 stage/sql/Init (stage) STATEMENT 104
106 106 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 105
Expand All @@ -230,7 +230,7 @@ select "With a third part to make things complete" as payload NULL NULL
117 117 stage/sql/Starting cleanup (stage) STATEMENT 104
118 118 stage/sql/Freeing items (stage) STATEMENT 104
119 119 wait/io/socket/sql/client_connection send STATEMENT 104
120 120 wait/synch/mutex/sql/THD::LOCK_thd_data lock STATEMENT 104
120 120 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STATEMENT 104
121 122 stage/sql/Reset for next command (stage) STATEMENT 104
122 122 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 121
123 123 idle idle NULL NULL
Expand All @@ -252,7 +252,7 @@ select "With a third part to make things complete" as payload NULL NULL
139 139 stage/sql/Starting cleanup (stage) STATEMENT 124
140 140 stage/sql/Freeing items (stage) STATEMENT 124
141 141 wait/io/socket/sql/client_connection send STATEMENT 124
142 142 wait/synch/mutex/sql/THD::LOCK_thd_data lock STATEMENT 124
142 142 wait/synch/mutex/sql/THD::LOCK_thd_kill lock STATEMENT 124
143 144 stage/sql/Reset for next command (stage) STATEMENT 124
144 144 wait/synch/mutex/sql/THD::LOCK_thd_data lock STAGE 143
disconnect con1;
1 change: 1 addition & 0 deletions mysql-test/suite/perfschema/t/nesting.test
Expand Up @@ -39,6 +39,7 @@ update performance_schema.setup_instruments set enabled='YES', timed='YES'
'wait/io/socket/sql/client_connection',
'wait/synch/rwlock/sql/LOCK_grant',
'wait/synch/mutex/sql/THD::LOCK_thd_data',
'wait/synch/mutex/sql/THD::LOCK_thd_kill',
'wait/io/file/sql/query_log');

update performance_schema.setup_instruments set enabled='YES', timed='YES'
Expand Down
3 changes: 0 additions & 3 deletions sql/event_scheduler.cc
Expand Up @@ -648,14 +648,11 @@ Event_scheduler::stop()
state= STOPPING;
DBUG_PRINT("info", ("Scheduler thread has id %lu",
(ulong) scheduler_thd->thread_id));
/* Lock from delete */
mysql_mutex_lock(&scheduler_thd->LOCK_thd_data);
/* This will wake up the thread if it waits on Queue's conditional */
sql_print_information("Event Scheduler: Killing the scheduler thread, "
"thread id %lu",
(ulong) scheduler_thd->thread_id);
scheduler_thd->awake(KILL_CONNECTION);
mysql_mutex_unlock(&scheduler_thd->LOCK_thd_data);

/* thd could be 0x0, when shutting down */
sql_print_information("Event Scheduler: "
Expand Down
24 changes: 12 additions & 12 deletions sql/my_apc.cc
Expand Up @@ -34,7 +34,7 @@
void Apc_target::init(mysql_mutex_t *target_mutex)
{
DBUG_ASSERT(!enabled);
LOCK_thd_data_ptr= target_mutex;
LOCK_thd_kill_ptr= target_mutex;
#ifndef DBUG_OFF
n_calls_processed= 0;
#endif
Expand All @@ -45,7 +45,7 @@ void Apc_target::init(mysql_mutex_t *target_mutex)

void Apc_target::enqueue_request(Call_request *qe)
{
mysql_mutex_assert_owner(LOCK_thd_data_ptr);
mysql_mutex_assert_owner(LOCK_thd_kill_ptr);
if (apc_calls)
{
Call_request *after= apc_calls->prev;
Expand All @@ -71,7 +71,7 @@ void Apc_target::enqueue_request(Call_request *qe)

void Apc_target::dequeue_request(Call_request *qe)
{
mysql_mutex_assert_owner(LOCK_thd_data_ptr);
mysql_mutex_assert_owner(LOCK_thd_kill_ptr);
if (apc_calls == qe)
{
if ((apc_calls= apc_calls->next) == qe)
Expand Down Expand Up @@ -145,14 +145,14 @@ bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call,

int wait_res= 0;
PSI_stage_info old_stage;
caller_thd->ENTER_COND(&apc_request.COND_request, LOCK_thd_data_ptr,
caller_thd->ENTER_COND(&apc_request.COND_request, LOCK_thd_kill_ptr,
&stage_show_explain, &old_stage);
/* todo: how about processing other errors here? */
while (!apc_request.processed && (wait_res != ETIMEDOUT))
{
/* We own LOCK_thd_data_ptr */
/* We own LOCK_thd_kill_ptr */
wait_res= mysql_cond_timedwait(&apc_request.COND_request,
LOCK_thd_data_ptr, &abstime);
LOCK_thd_kill_ptr, &abstime);
// &apc_request.LOCK_request, &abstime);
if (caller_thd->killed)
break;
Expand All @@ -163,7 +163,7 @@ bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call,
/*
The wait has timed out, or this thread was KILLed.
Remove the request from the queue (ok to do because we own
LOCK_thd_data_ptr)
LOCK_thd_kill_ptr)
*/
apc_request.processed= TRUE;
dequeue_request(&apc_request);
Expand All @@ -176,7 +176,7 @@ bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call,
res= FALSE;
}
/*
exit_cond() will call mysql_mutex_unlock(LOCK_thd_data_ptr) for us:
exit_cond() will call mysql_mutex_unlock(LOCK_thd_kill_ptr) for us:
*/
caller_thd->EXIT_COND(&old_stage);

Expand All @@ -185,7 +185,7 @@ bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call,
}
else
{
mysql_mutex_unlock(LOCK_thd_data_ptr);
mysql_mutex_unlock(LOCK_thd_kill_ptr);
}
return res;
}
Expand All @@ -202,11 +202,11 @@ void Apc_target::process_apc_requests()
{
Call_request *request;

mysql_mutex_lock(LOCK_thd_data_ptr);
mysql_mutex_lock(LOCK_thd_kill_ptr);
if (!(request= get_first_in_queue()))
{
/* No requests in the queue */
mysql_mutex_unlock(LOCK_thd_data_ptr);
mysql_mutex_unlock(LOCK_thd_kill_ptr);
break;
}

Expand All @@ -225,7 +225,7 @@ void Apc_target::process_apc_requests()
n_calls_processed++;
#endif
mysql_cond_signal(&request->COND_request);
mysql_mutex_unlock(LOCK_thd_data_ptr);
mysql_mutex_unlock(LOCK_thd_kill_ptr);
}
}

6 changes: 3 additions & 3 deletions sql/my_apc.h
Expand Up @@ -44,7 +44,7 @@ class THD;
*/
class Apc_target
{
mysql_mutex_t *LOCK_thd_data_ptr;
mysql_mutex_t *LOCK_thd_kill_ptr;
public:
Apc_target() : enabled(0), apc_calls(NULL) {}
~Apc_target() { DBUG_ASSERT(!enabled && !apc_calls);}
Expand All @@ -66,9 +66,9 @@ class Apc_target
void disable()
{
DBUG_ASSERT(enabled);
mysql_mutex_lock(LOCK_thd_data_ptr);
mysql_mutex_lock(LOCK_thd_kill_ptr);
bool process= !--enabled && have_apc_requests();
mysql_mutex_unlock(LOCK_thd_data_ptr);
mysql_mutex_unlock(LOCK_thd_kill_ptr);
if (unlikely(process))
process_apc_requests();
}
Expand Down
4 changes: 2 additions & 2 deletions sql/mysqld.cc
Expand Up @@ -1704,7 +1704,7 @@ static void close_connections(void)
#endif
tmp->set_killed(KILL_SERVER_HARD);
MYSQL_CALLBACK(thread_scheduler, post_kill_notification, (tmp));
mysql_mutex_lock(&tmp->LOCK_thd_data);
mysql_mutex_lock(&tmp->LOCK_thd_kill);
if (tmp->mysys_var)
{
tmp->mysys_var->abort=1;
Expand All @@ -1727,7 +1727,7 @@ static void close_connections(void)
}
mysql_mutex_unlock(&tmp->mysys_var->mutex);
}
mysql_mutex_unlock(&tmp->LOCK_thd_data);
mysql_mutex_unlock(&tmp->LOCK_thd_kill);
}
mysql_mutex_unlock(&LOCK_thread_count); // For unlink from list

Expand Down
8 changes: 3 additions & 5 deletions sql/slave.cc
Expand Up @@ -343,9 +343,7 @@ handle_slave_background(void *arg __attribute__((unused)))
THD *to_kill= p->to_kill;
kill_list= p->next;

mysql_mutex_lock(&to_kill->LOCK_thd_data);
to_kill->awake(KILL_CONNECTION);
mysql_mutex_unlock(&to_kill->LOCK_thd_data);
mysql_mutex_lock(&to_kill->LOCK_wakeup_ready);
to_kill->rgi_slave->killed_for_retry=
rpl_group_info::RETRY_KILL_KILLED;
Expand Down Expand Up @@ -856,7 +854,7 @@ terminate_slave_thread(THD *thd,
int error __attribute__((unused));
DBUG_PRINT("loop", ("killing slave thread"));

mysql_mutex_lock(&thd->LOCK_thd_data);
mysql_mutex_lock(&thd->LOCK_thd_kill);
#ifndef DONT_USE_THR_ALARM
/*
Error codes from pthread_kill are:
Expand All @@ -866,9 +864,9 @@ terminate_slave_thread(THD *thd,
int err __attribute__((unused))= pthread_kill(thd->real_id, thr_client_alarm);
DBUG_ASSERT(err != EINVAL);
#endif
thd->awake(NOT_KILLED);
thd->awake_no_mutex(NOT_KILLED);

mysql_mutex_unlock(&thd->LOCK_thd_data);
mysql_mutex_unlock(&thd->LOCK_thd_kill);

/*
There is a small chance that slave thread might miss the first
Expand Down

0 comments on commit c2118a0

Please sign in to comment.