Re: [PATCH v4 2/2] rcu: add uninit destructor for rcu

[Stefan Hajnoczi]

On Tue, Sep 08, 2020 at 11:10:52PM +0800, Yonggang Luo wrote:
> This is necessary if the pending  rcu calls are closing and removing
> temp files. This also provide a function
> void rcu_wait_finished(void);
> to fixes test-logging.c test failure on msys2/mingw.
> On windows if the file doesn't closed, you can not remove it.
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> ---
>  include/qemu/rcu.h   |  5 +++++
>  tests/test-logging.c |  2 ++
>  util/rcu.c           | 37 ++++++++++++++++++++++++++++++++++++-
>  3 files changed, 43 insertions(+), 1 deletion(-)

Can the new drain_call_rcu() function be used? Maxim recently posted the
following patch:
https://patchew.org/QEMU/20200831150124.206267-1-mlevitsk@redhat.com/20200831150124.206267-3-mlevitsk@redhat.com/

Whether drain_call_rcu() or rcu_wait_finished() is used, please include
a comment in the code that documents why the wait is necessary. For
example, "qemu_log_close() uses RCU for its FILE pointer but Windows
cannot remove open files, so we need to wait for RCU here".

Another option is to wait for RCU inside qemu_log_close() so that
callers don't need to worry about this implementation detail:

  #ifdef _WIN32
  /* Windows cannot remove open files so we need to wait for RCU here */
  drain_call_rcu();
  #endif

> diff --git a/include/qemu/rcu.h b/include/qemu/rcu.h
> index 570aa603eb..dd0a92c1d0 100644
> --- a/include/qemu/rcu.h
> +++ b/include/qemu/rcu.h
> @@ -124,6 +124,11 @@ extern void rcu_unregister_thread(void);
>  extern void rcu_enable_atfork(void);
>  extern void rcu_disable_atfork(void);
>  
> +/*
> + * Wait all rcu call executed and exit the rcu thread.
> + */
> +extern void rcu_wait_finished(void);
> +
>  struct rcu_head;
>  typedef void RCUCBFunc(struct rcu_head *head);
>  
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index 957f6c08cd..7a5b59f4a5 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -210,6 +210,8 @@ int main(int argc, char **argv)
>                           tmp_path, test_logfile_lock);
>  
>      rc = g_test_run();
> +    qemu_log_close();
> +    rcu_wait_finished();
>  
>      rmdir_full(tmp_path);
>      g_free(tmp_path);
> diff --git a/util/rcu.c b/util/rcu.c
> index 60a37f72c3..43367988b9 100644
> --- a/util/rcu.c
> +++ b/util/rcu.c
> @@ -308,10 +308,20 @@ void rcu_unregister_thread(void)
>      qemu_mutex_unlock(&rcu_registry_lock);
>  }
>  
> +typedef struct QemuRcuMessage {
> +    struct rcu_head rcu;
> +    void *message;
> +} QemuRcuMessage;
> +
> +static int rcu_thread_exit_called = 0;
> +static int rcu_thread_exited = 0;
> +static QemuRcuMessage rcu_thread_message;
> +
>  static void rcu_init_complete(void)
>  {
>      QemuThread thread;
> -
> +    atomic_mb_set(&rcu_thread_exit_called, 0);
> +    atomic_mb_set(&rcu_thread_exited, 0);
>      qemu_mutex_init(&rcu_registry_lock);
>      qemu_mutex_init(&rcu_sync_lock);
>      qemu_event_init(&rcu_gp_event, true);
> @@ -327,6 +337,26 @@ static void rcu_init_complete(void)
>      rcu_register_thread();
>  }
>  
> +static void rcu_thread_exit(QemuRcuMessage *param)
> +{
> +    atomic_mb_set((int*)param->message, 1);
> +    qemu_thread_exit(NULL);
> +}
> +
> +void rcu_wait_finished(void)
> +{
> +    if (atomic_xchg(&rcu_thread_exit_called, 1) == 0)
> +    {
> +        rcu_thread_message.message = &rcu_thread_exited;
> +        call_rcu(&rcu_thread_message, rcu_thread_exit, rcu);
> +    }
> +
> +    while (atomic_mb_read(&rcu_thread_exited) == 0)
> +    {
> +        g_usleep(10000);
> +    }
> +}
> +
>  static int atfork_depth = 1;
>  
>  void rcu_enable_atfork(void)
> @@ -379,3 +409,8 @@ static void __attribute__((__constructor__)) 
> rcu_init(void)
>  #endif
>      rcu_init_complete();
>  }
> +
> +static void __attribute__((__destructor__)) rcu_uninit(void)
> +{
> +    rcu_wait_finished();
> +}
> -- 
> 2.28.0.windows.1
>

signature.asc
Description: PGP signature