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 <holyfoot(a)mariadb.com>
> committer: Alexey Botchkov <holyfoot(a)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 security(a)mariadb.org
