[PATCH v4 0/2] rcu: fixes rcu and test-logging.c

[PATCH v4 0/2] rcu: fixes rcu and test-logging.c

[Yonggang Luo]

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 failures on msys2/mingw.
On windows if the file doesn't closed, you can not remove it.

Yonggang Luo (2):
  logging: Fixes memory leak in test-logging.c
  rcu: add uninit destructor for rcu

 include/qemu/rcu.h   |  5 +++++
 tests/test-logging.c |  4 +++-
 util/rcu.c           | 37 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 44 insertions(+), 2 deletions(-)

-- 
2.28.0.windows.1

[PATCH v4 1/2] logging: Fixes memory leak in test-logging.c

[Yonggang Luo]

g_dir_make_tmp Returns the actual name used. This string should be
freed with g_free() when not needed any longer and is is in the GLib
file name encoding. In case of errors, NULL is returned and error will
be set. Use g_autofree to free it properly

Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/test-logging.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/test-logging.c b/tests/test-logging.c
index 8a1161de1d..957f6c08cd 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -196,7 +196,7 @@ static void rmdir_full(gchar const *root)
 
 int main(int argc, char **argv)
 {
-    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
+    g_autofree gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
     int rc;
 
     g_test_init(&argc, &argv, NULL);
-- 
2.28.0.windows.1

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

[Yonggang Luo]

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(-)

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

Re: [PATCH v4 1/2] logging: Fixes memory leak in test-logging.c

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1153 bytes --]

On Tue, Sep 08, 2020 at 11:10:51PM +0800, Yonggang Luo wrote:
> g_dir_make_tmp Returns the actual name used. This string should be
> freed with g_free() when not needed any longer and is is in the GLib
> file name encoding. In case of errors, NULL is returned and error will
> be set. Use g_autofree to free it properly
> 
> Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/test-logging.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index 8a1161de1d..957f6c08cd 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -196,7 +196,7 @@ static void rmdir_full(gchar const *root)
>  
>  int main(int argc, char **argv)
>  {
> -    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
> +    g_autofree gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
>      int rc;
>  
>      g_test_init(&argc, &argv, NULL);

I don't see the memory leak. There is a g_free(tmp_path) at the bottom
of main().

Did I miss something?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4 1/2] logging: Fixes memory leak in test-logging.c

[罗勇刚(Yonggang Luo)]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Wed, Sep 9, 2020 at 4:30 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, Sep 08, 2020 at 11:10:51PM +0800, Yonggang Luo wrote:
> > g_dir_make_tmp Returns the actual name used. This string should be
> > freed with g_free() when not needed any longer and is is in the GLib
> > file name encoding. In case of errors, NULL is returned and error will
> > be set. Use g_autofree to free it properly
> >
> > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  tests/test-logging.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tests/test-logging.c b/tests/test-logging.c
> > index 8a1161de1d..957f6c08cd 100644
> > --- a/tests/test-logging.c
> > +++ b/tests/test-logging.c
> > @@ -196,7 +196,7 @@ static void rmdir_full(gchar const *root)
> >
> >  int main(int argc, char **argv)
> >  {
> > -    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
> > +    g_autofree gchar *tmp_path =
> g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
> >      int rc;
> >
> >      g_test_init(&argc, &argv, NULL);
>
> I don't see the memory leak. There is a g_free(tmp_path) at the bottom
> of main().
>
> Did I miss something?
>
Oh, gocha, this issue fixed by someone else. So when I rebasing, something
are lost.
 I am intent replace the free with  g_autofree , should I update it? this
is not a fix anymore, just
a improve

>
> Stefan
>


-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 2454 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4002 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[罗勇刚(Yonggang Luo)]

[-- Attachment #1: Type: text/plain, Size: 4935 bytes --]

On Wed, Sep 9, 2020 at 4:41 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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
>
How about not gurad with   #ifdef _WIN32?
So we don't got silent differencies between posix and win32?
and qemu_log_close  only called in function cpu_abort()
      if (qemu_log_separate()) {
        FILE *logfile = qemu_log_lock();
        qemu_log("qemu: fatal: ");
        qemu_log_vprintf(fmt, ap2);
        qemu_log("\n");
        log_cpu_state(cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
        qemu_log_flush();
        qemu_log_unlock(logfile);
        qemu_log_close();
    }

So that on't affect the performance

>
>
> > 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
> >
>


-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 6611 bytes --]

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

[罗勇刚(Yonggang Luo)]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]

On Wed, Sep 9, 2020 at 4:41 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> 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();
>
once  drain_call_rcu  function are merged, I will convert the patch to use
it.

>   #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
> >
>


-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 6016 bytes --]

Re: [PATCH v4 1/2] logging: Fixes memory leak in test-logging.c

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1859 bytes --]

On Wed, Sep 09, 2020 at 04:35:26PM +0800, 罗勇刚(Yonggang Luo) wrote:
> On Wed, Sep 9, 2020 at 4:30 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > On Tue, Sep 08, 2020 at 11:10:51PM +0800, Yonggang Luo wrote:
> > > g_dir_make_tmp Returns the actual name used. This string should be
> > > freed with g_free() when not needed any longer and is is in the GLib
> > > file name encoding. In case of errors, NULL is returned and error will
> > > be set. Use g_autofree to free it properly
> > >
> > > Signed-off-by: Yonggang Luo <luoyonggang@gmail.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > ---
> > >  tests/test-logging.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tests/test-logging.c b/tests/test-logging.c
> > > index 8a1161de1d..957f6c08cd 100644
> > > --- a/tests/test-logging.c
> > > +++ b/tests/test-logging.c
> > > @@ -196,7 +196,7 @@ static void rmdir_full(gchar const *root)
> > >
> > >  int main(int argc, char **argv)
> > >  {
> > > -    gchar *tmp_path = g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
> > > +    g_autofree gchar *tmp_path =
> > g_dir_make_tmp("qemu-test-logging.XXXXXX", NULL);
> > >      int rc;
> > >
> > >      g_test_init(&argc, &argv, NULL);
> >
> > I don't see the memory leak. There is a g_free(tmp_path) at the bottom
> > of main().
> >
> > Did I miss something?
> >
> Oh, gocha, this issue fixed by someone else. So when I rebasing, something
> are lost.
>  I am intent replace the free with  g_autofree , should I update it? this
> is not a fix anymore, just
> a improve

If you want. It's not essential in this function since there aren't
return statements where memory leaks often occur, but since you're
already working on the code, it's still an improvement to use
g_autofree.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

On Wed, Sep 09, 2020 at 05:05:16PM +0800, 罗勇刚(Yonggang Luo) wrote:
> On Wed, Sep 9, 2020 at 4:41 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > 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
> >
> How about not gurad with   #ifdef _WIN32?
> So we don't got silent differencies between posix and win32?
> and qemu_log_close  only called in function cpu_abort()
>       if (qemu_log_separate()) {
>         FILE *logfile = qemu_log_lock();
>         qemu_log("qemu: fatal: ");
>         qemu_log_vprintf(fmt, ap2);
>         qemu_log("\n");
>         log_cpu_state(cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
>         qemu_log_flush();
>         qemu_log_unlock(logfile);
>         qemu_log_close();
>     }
> 
> So that on't affect the performance

Yes, that sounds good.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Paolo Bonzini]

[-- Attachment #1.1: Type: text/plain, Size: 1485 bytes --]

On 09/09/20 10:41, Stefan Hajnoczi wrote:
> 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
> 

In this case even synchronize_rcu() should be okay.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[罗勇刚(Yonggang Luo)]

[-- Attachment #1: Type: text/plain, Size: 1739 bytes --]

On Thu, Sep 10, 2020 at 1:23 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 09/09/20 10:41, Stefan Hajnoczi wrote:
> > 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
> >
>
> In this case even synchronize_rcu() should be okay.
>
> Tried and not working.

> Paolo
>
>

-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 2745 bytes --]