| Message ID | 20200908151052.713-3-luoyonggang@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rcu: fixes rcu and test-logging.c | expand |
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 >
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 > > >
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 > > >
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
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
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 > >
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(); +}
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(-)