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 <holyf...@mariadb.com> > committer: Alexey Botchkov <holyf...@mariadb.com> > 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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp