Re: [Maria-developers] 19bdb03e97d: MDEV-21211 LOCK_plugin optimization.

Hi, Alexey! Thanks, this is great! Comments are below, there aren't many. The main concern - you only do RCU when installingplugins, but not when uninstalling. I'm not sure how it's supposed to work. On Oct 02, Alexey Botchkov wrote:

revision-id: 19bdb03e97d (mariadb-10.4.11-337-g19bdb03e97d) parent(s): 0041dacc1b8 author: Alexey Botchkov committer: Alexey Botchkov timestamp: 2020-08-11 00:01:53 +0400 message:

MDEV-21211 LOCK_plugin optimization.

Use Apc_target calls to 'notify' threads about changes in plugins.

diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index bf33148811e..1f38c55b922 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -1348,10 +1348,10 @@ bool do_command(THD *thd) my_net_set_read_timeout(net, thd->variables.net_read_timeout);

DBUG_ASSERT(packet_length); - DBUG_ASSERT(!thd->apc_target.is_enabled()); + thd->apc_target.enable(); return_value= dispatch_command(command, thd, packet+1, (uint) (packet_length-1), FALSE, FALSE); - DBUG_ASSERT(!thd->apc_target.is_enabled()); + thd->apc_target.disable();

is this because you don't want to post apc requests to threads that wait for the client?

out: thd->lex->restore_set_statement_var(); diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 981420f03ae..84438e3ff70 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -217,17 +217,28 @@ static struct #include "sql_plugin_services.ic"

/* - A mutex LOCK_plugin must be acquired before accessing the - following variables/structures. - We are always manipulating ref count, so a rwlock here is unneccessary. + A mutex LOCK_plugin must be acquired when we change the + new_plugins_state content and when we swap the global_plugins_state + and the new_plugins_state. */ mysql_mutex_t LOCK_plugin; +struct st_plugins_state +{ + DYNAMIC_ARRAY m_array; + HASH m_hash[MYSQL_MAX_PLUGIN_TYPE_NUM]; +#ifndef DBUG_OOF

DBUG_OFF

+ uint m_ref_counter; +#endif +}; + static DYNAMIC_ARRAY plugin_dl_array; -static DYNAMIC_ARRAY plugin_array; -static HASH plugin_hash[MYSQL_MAX_PLUGIN_TYPE_NUM]; static MEM_ROOT plugin_mem_root; static bool reap_needed= false; volatile int global_plugin_version= 1; +static st_plugins_state plugins_states[2]; +st_plugins_state *global_plugins_state= plugins_states;

it seems this can be static too, you don't use it outside of sql_plugin.cc (and making it global_plugins_state= &plugins_states[0], new_plugins_state= &plugins_state[1]; would be a bit more explicit)

+static st_plugins_state *new_plugins_state= plugins_states + 1; +

static bool initialized= 0; ulong dlopen_count; @@ -871,7 +882,8 @@ static void plugin_dl_del(struct st_plugin_dl *plugin_dl) }

-static struct st_plugin_int *plugin_find_internal(const LEX_CSTRING *name, +static struct st_plugin_int *plugin_find_internal(st_plugins_state *st,

shouldn't `st` be const here too?

+ const LEX_CSTRING *name, int type) { uint i; @@ -879,21 +891,19 @@ static struct st_plugin_int *plugin_find_internal(const LEX_CSTRING *name, if (! initialized) DBUG_RETURN(0);

- mysql_mutex_assert_owner(&LOCK_plugin);

may be you'd want here something like if (st == new_plugins_state) mysql_mutex_assert_owner(&LOCK_plugin);

- if (type == MYSQL_ANY_PLUGIN) { for (i= 0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) { struct st_plugin_int *plugin= (st_plugin_int *) - my_hash_search(&plugin_hash[i], (const uchar *)name->str, name->length); + my_hash_search(&st->m_hash[i], (const uchar *)name->str, name->length); if (plugin) DBUG_RETURN(plugin); } } else DBUG_RETURN((st_plugin_int *) - my_hash_search(&plugin_hash[type], (const uchar *)name->str, + my_hash_search(&st->m_hash[type], (const uchar *)name->str, name->length)); DBUG_RETURN(0); } @@ -1210,13 +1209,6 @@ static void plugin_variables_deinit(struct st_plugin_int *plugin)

static void plugin_deinitialize(struct st_plugin_int *plugin, bool ref_check) { - /* - we don't want to hold the LOCK_plugin mutex as it may cause - deinitialization to deadlock if plugins have worker threads - with plugin locks - */ - mysql_mutex_assert_not_owner(&LOCK_plugin);

does it mean you *do* keep LOCK_plugin here now?

- if (plugin->plugin->status_vars) { /* @@ -1280,24 +1273,25 @@ static void plugin_del(struct st_plugin_int *plugin) DBUG_VOID_RETURN; }

+ static void reap_plugins(void) { uint count; struct st_plugin_int *plugin, **reap, **list;

- mysql_mutex_assert_owner(&LOCK_plugin); - if (!reap_needed) return;

+ mysql_mutex_lock(&LOCK_plugin); reap_needed= false; - count= plugin_array.elements; + count= global_plugins_state->m_array.elements; reap= (struct st_plugin_int **)my_alloca(sizeof(plugin)*(count+1)); *(reap++)= NULL;

for (uint i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) { - HASH *hash= plugin_hash + plugin_type_initialization_order[i]; + HASH *hash= global_plugins_state->m_hash + + plugin_type_initialization_order[i];

do you need LOCK_plugin when changing plugin states? your comment at the LOCK_plugin declaration doesn't mention it.

for (uint j= 0; j < hash->records; j++) { plugin= (struct st_plugin_int *) my_hash_element(hash, j); @@ -1542,6 +1529,38 @@ static void init_plugin_psi_keys(void) } #endif /* HAVE_PSI_INTERFACE */

+ +static bool copy_plugins_state(st_plugins_state *dest, st_plugins_state *src) +{ + uint i; +

mysql_mutex_assert_owner ? for documentation purposes.

+ reset_dynamic(&dest->m_array); + for (i= 0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) + my_hash_reset(&dest->m_hash[i]); + + for (i= 0; i < src->m_array.elements; i++) + { + struct st_plugin_int *ptr; + ptr= *dynamic_element(&src->m_array, i, struct st_plugin_int **); + if (insert_dynamic(&dest->m_array, (uchar *) &ptr) || + (ptr->state != PLUGIN_IS_FREED && + my_hash_insert(&dest->m_hash[ptr->plugin->type], + (uchar*) ptr))) + goto err; + } + + return false; +err: + return true; +} + + +static void set_new_global_plugins_state() +{

mysql_mutex_assert_owner ? for documentation purposes.

+ swap_variables(st_plugins_state *, global_plugins_state, new_plugins_state); +} + + /* The logic is that we first load and initialize all compiled in plugins. From there we load up the dynamic types (assuming we have not been told to @@ -1815,7 +1834,9 @@ static void plugin_load(MEM_ROOT *tmp_root) tables.init_one_table(&MYSQL_SCHEMA_NAME, &MYSQL_PLUGIN_NAME, 0, TL_READ); tables.open_strategy= TABLE_LIST::OPEN_NORMAL;

+ set_new_global_plugins_state(); result= open_and_lock_tables(new_thd, &tables, FALSE, MYSQL_LOCK_IGNORE_TIMEOUT); + set_new_global_plugins_state();

why here, why twice?

table= tables.table; if (result) @@ -1851,52 +1872,31 @@ static void plugin_load(MEM_ROOT *tmp_root) if (!name.length || !dl.length) continue;

- /* - Pre-acquire audit plugins for events that may potentially occur - during [UN]INSTALL PLUGIN. - - When audit event is triggered, audit subsystem acquires interested - plugins by walking through plugin list. Evidently plugin list - iterator protects plugin list by acquiring LOCK_plugin, see - plugin_foreach_with_mask(). - - On the other hand plugin_load is acquiring LOCK_plugin - rather for a long time. - - When audit event is triggered during plugin_load plugin - list iterator acquires the same lock (within the same thread) - second time. - - This hack should be removed when LOCK_plugin is fixed so it - protects only what it supposed to protect. - - See also mysql_install_plugin(), mysql_uninstall_plugin() and - initialize_audit_plugin() - */ - if (mysql_audit_general_enabled()) - mysql_audit_acquire_plugins(new_thd, event_class_mask); -

\o/

/* there're no other threads running yet, so we don't need a mutex. but plugin_add() before is designed to work in multi-threaded environment, and it uses mysql_mutex_assert_owner(), so we lock the mutex here to satisfy the assert */ - mysql_mutex_lock(&LOCK_plugin); plugin_add(tmp_root, false, &name, &dl, MYF(ME_ERROR_LOG)); free_root(tmp_root, MYF(MY_MARK_BLOCKS_FREE)); - mysql_mutex_unlock(&LOCK_plugin); } if (unlikely(error > 0)) sql_print_error(ER_THD(new_thd, ER_GET_ERRNO), my_errno, table->file->table_type()); + set_new_global_plugins_state(); end_read_record(&read_record_info); table->mark_table_for_reopen(); close_mysql_tables(new_thd); -end: +err_ret: new_thd->db= null_clex_str; // Avoid free on thd->db delete new_thd; + set_new_global_plugins_state(); DBUG_VOID_RETURN; + +end: + set_new_global_plugins_state(); + goto err_ret;

eh, really? you swap twice again?

}

@@ -2058,13 +2055,8 @@ void plugin_shutdown(void) plugin_deinitialize(plugins[i], false); }

- /* - It's perfectly safe not to lock LOCK_plugin, as there're no - concurrent threads anymore. But some functions called from here - use mysql_mutex_assert_owner(), so we lock the mutex to satisfy it - */ - mysql_mutex_lock(&LOCK_plugin);

+ mysql_mutex_lock(&LOCK_plugin);

the comment was helpful, I think

/* We defer checking ref_counts until after all plugins are deinitialized as some may have worker threads holding on to plugin references. @@ -2176,6 +2173,44 @@ static bool finalize_install(THD *thd, TABLE *table, const LEX_CSTRING *name, return 0; }

+ +/* + Request object for ending the new_plugins_state change. +*/ + +class Ping_all_threads_request : public Apc_target::Apc_call +{ +public: + /* Overloaded virtual functions. */ + void call_in_target_thread(); +}; + + +void Ping_all_threads_request::call_in_target_thread() +{ + /* Do nothing. We just make sure the thread got to this point. */ +} + + +static my_bool fix_plugins_state_callback(THD *thd, THD *caller_thd) +{ + bool timed_out; + int timeout_sec= 30; + + Ping_all_threads_request ping_req; + + mysql_mutex_lock(&thd->LOCK_thd_kill); + if (thd->get_command() == COM_SLEEP)

here you also skip threads waiting on the client?

+ { + mysql_mutex_unlock(&thd->LOCK_thd_kill); + return FALSE; + } + thd->apc_target.make_apc_call(caller_thd, &ping_req, + timeout_sec, &timed_out); + return FALSE; +} + + bool mysql_install_plugin(THD *thd, const LEX_CSTRING *name, const LEX_CSTRING *dl_arg) { @@ -2252,12 +2272,19 @@ bool mysql_install_plugin(THD *thd, const LEX_CSTRING *name,

if (unlikely(error != INSTALL_GOOD)) { + mysql_mutex_unlock(&LOCK_plugin); reap_needed= true; reap_plugins(); + goto err; } -err: global_plugin_version++; + set_new_global_plugins_state(); + thd->apc_target.disable(); mysql_mutex_unlock(&LOCK_plugin); + server_threads.iterate(fix_plugins_state_callback, thd); + thd->apc_target.enable();

a comment be good here. to explain why you disable apc and why disable/unlock/iterate must happen in this particlar order.

+ +err: if (argv) free_defaults(argv); DBUG_RETURN(error == INSTALL_FAIL_NOT_OK); @@ -2414,10 +2416,12 @@ bool mysql_uninstall_plugin(THD *thd, const LEX_CSTRING *name, error|= !MyFlags; } } - reap_plugins(); - global_plugin_version++; + thd->apc_target.disable(); mysql_mutex_unlock(&LOCK_plugin); + server_threads.iterate(fix_plugins_state_callback, thd); + reap_plugins(); + thd->apc_target.enable();

I don't quite understand this. You don't do copy-update here. I presume because plugins are only marked PLUGIN_IS_FREED but not actually removed from the array or hash? but you modify plugin state under a mutex, and read it without a mutex. how is that supposed to work?

DBUG_RETURN(error); #ifdef WITH_WSREP wsrep_error_label:

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org