maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> committer: Alexey Botchkov <holyfoot@xxxxxxxxxxx>
> 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@xxxxxxxxxxx