The function is called only at tcg_gen_code() when duplicated TBs
are translated by different threads, and when the tcg_region_tree
is reset. Bake it into the underlying GTree as its value destroy
function to unite these situations.
Also remove tcg_region_tree_traverse() which now becomes useless.

Signed-off-by: Liren Wei <lrwei@bupt.edu.cn>
---
 accel/tcg/translate-all.c |  6 ------
 include/tcg/tcg.h         |  1 -
 tcg/region.c              | 18 +++++++-----------
 3 files changed, 7 insertions(+), 18 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 75e4d06557..57455d8639 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -378,11 +378,6 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
     return 0;
 }
 
-void tb_destroy(TranslationBlock *tb)
-{
-    qemu_spin_destroy(&tb->jmp_lock);
-}
-
 bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
 {
     /*
@@ -1681,7 +1676,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
         orig_aligned -= ROUND_UP(sizeof(*tb), qemu_icache_linesize);
         qatomic_set(&tcg_ctx->code_gen_ptr, (void *)orig_aligned);
-        tb_destroy(tb);
         tcg_tb_remove(tb);
         return existing_tb;
     }
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 899493701c..dedb86939a 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -808,7 +808,6 @@ void *tcg_malloc_internal(TCGContext *s, int size);
 void tcg_pool_reset(TCGContext *s);
 TranslationBlock *tcg_tb_alloc(TCGContext *s);
 
-void tb_destroy(TranslationBlock *tb);
 void tcg_region_reset_all(void);
 
 size_t tcg_code_size(void);
diff --git a/tcg/region.c b/tcg/region.c
index 00b0c3b091..956a5ae483 100644
--- a/tcg/region.c
+++ b/tcg/region.c
@@ -112,7 +112,7 @@ static int ptr_cmp_tb_tc(const void *ptr, const struct tb_tc *s)
     return 0;
 }
 
-static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
+static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp, gpointer _)
 {
     const struct tb_tc *a = ap;
     const struct tb_tc *b = bp;
@@ -143,6 +143,11 @@ static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
     return ptr_cmp_tb_tc(b->ptr, a);
 }
 
+static void tb_destroy(gpointer tb)
+{
+    qemu_spin_destroy(&((TranslationBlock *) tb)->jmp_lock);
+}
+
 static void tcg_region_trees_init(void)
 {
     size_t i;
@@ -153,7 +158,7 @@ static void tcg_region_trees_init(void)
         struct tcg_region_tree *rt = region_trees + i * tree_size;
 
         qemu_mutex_init(&rt->lock);
-        rt->tree = g_tree_new(tb_tc_cmp);
+        rt->tree = g_tree_new_full(tb_tc_cmp, NULL, NULL, tb_destroy);
     }
 }
 
@@ -277,14 +282,6 @@ size_t tcg_nb_tbs(void)
     return nb_tbs;
 }
 
-static gboolean tcg_region_tree_traverse(gpointer k, gpointer v, gpointer data)
-{
-    TranslationBlock *tb = v;
-
-    tb_destroy(tb);
-    return FALSE;
-}
-
 static void tcg_region_tree_reset_all(void)
 {
     size_t i;
@@ -293,7 +290,6 @@ static void tcg_region_tree_reset_all(void)
     for (i = 0; i < region.n; i++) {
         struct tcg_region_tree *rt = region_trees + i * tree_size;
 
-        g_tree_foreach(rt->tree, tcg_region_tree_traverse, NULL);
         /* Increment the refcount first so that destroy acts as a reset */
         g_tree_ref(rt->tree);
         g_tree_destroy(rt->tree);
-- 
2.32.0

On 7/4/21 7:31 AM, Liren Wei wrote:
> -static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
> +static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp, gpointer _)

Using _ here as the variable name isn't ideal.  I guess if this were c++ we would actually 
omit the name, which is kinda the same.  But I think it's just as easy to name it 
userdata, as per glib docs.

I'll fix that up while queuing, thanks.

I'm not keen that the spinlock init and destroy are in different places, but surely that 
should be fixed by moving the init to tcg_tb_alloc, probably moving it to tcg/region.c as 
well.


r~

On 7/7/21 8:34 AM, Richard Henderson wrote:
> On 7/4/21 7:31 AM, Liren Wei wrote:
>> -static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp)
>> +static gint tb_tc_cmp(gconstpointer ap, gconstpointer bp, gpointer _)
>
> Using _ here as the variable name isn't ideal.  I guess if this were 
> c++ we would actually omit the name, which is kinda the same.  But I 
> think it's just as easy to name it userdata, as per glib docs.
>
> I'll fix that up while queuing, thanks.
>
Got it, thanks.
> I'm not keen that the spinlock init and destroy are in different 
> places, but surely that should be fixed by moving the init to 
> tcg_tb_alloc, probably moving it to tcg/region.c as well.
>
>
> r~
Indeed, that would be much more clear. But I kind of feel that 
initialization of TB spinlock is deliberately placed after 
tcg_gen_code() in the current implementation to prevent buffer_overflow 
or any rewinding from leaking the initialized spinlock (, through it 
seems to me that there is nothing to leak for a spinlock whatsoever).

Liren Wei