[PATCH v14 07/10] tb-stats: reset the tracked TBs on a tb_flush

Fei Wu posted 10 patches 2 years, 8 months ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, "Dr. David Alan Gilbert" <dave@treblig.org>, Laurent Vivier <laurent@vivier.eu>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Thomas Huth <thuth@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Markus Armbruster <armbru@redhat.com>
There is a newer version of this series
[PATCH v14 07/10] tb-stats: reset the tracked TBs on a tb_flush
Posted by Fei Wu 2 years, 8 months ago
From: Alex Bennée <alex.bennee@linaro.org>

We keep track of translations but can only do so up until the
translation cache is flushed. At that point we really have no idea if
we can re-create a translation because all the active tracking
information has been reset.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Fei Wu <fei2.wu@intel.com>
---
 accel/tcg/tb-maint.c    |  1 +
 accel/tcg/tb-stats.c    | 19 +++++++++++++++++++
 include/exec/tb-stats.h |  8 ++++++++
 3 files changed, 28 insertions(+)

diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index 0980fca358..11ff0ddd90 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -763,6 +763,7 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
     qht_reset_size(&tb_ctx.htable, CODE_GEN_HTABLE_SIZE);
     tb_remove_all();
 
+    tbstats_reset_tbs();
     tcg_region_reset_all();
     /* XXX: flush processor icache at this point if cache flush is expensive */
     qatomic_inc(&tb_ctx.tb_flush_count);
diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
index 805e1fc74d..139f049ffc 100644
--- a/accel/tcg/tb-stats.c
+++ b/accel/tcg/tb-stats.c
@@ -267,6 +267,25 @@ void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd)
     g_free(cmdinfo);
 }
 
+/*
+ * We have to reset the tbs array on a tb_flush as those
+ * TranslationBlocks no longer exist and we no loner know if the
+ * current mapping is still valid.
+ */
+
+static void reset_tbs_array(void *p, uint32_t hash, void *userp)
+{
+    TBStatistics *tbs = p;
+    g_ptr_array_set_size(tbs->tbs, 0);
+}
+
+void tbstats_reset_tbs(void)
+{
+    if (tb_ctx.tb_stats.map) {
+        qht_iter(&tb_ctx.tb_stats, reset_tbs_array, NULL);
+    }
+}
+
 void init_tb_stats_htable(void)
 {
     if (!tb_ctx.tb_stats.map && tb_stats_collection_enabled()) {
diff --git a/include/exec/tb-stats.h b/include/exec/tb-stats.h
index 4bb343870b..30b788f7b2 100644
--- a/include/exec/tb-stats.h
+++ b/include/exec/tb-stats.h
@@ -124,4 +124,12 @@ struct TbstatsCommand {
 
 void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd);
 
+/**
+ * tbstats_reset_tbs: reset the linked array of TBs
+ *
+ * Reset the list of tbs for a given array. Should be called from
+ * safe work during tb_flush.
+ */
+void tbstats_reset_tbs(void);
+
 #endif
-- 
2.25.1


Re: [PATCH v14 07/10] tb-stats: reset the tracked TBs on a tb_flush
Posted by Richard Henderson 2 years, 8 months ago
On 5/30/23 01:35, Fei Wu wrote:
> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
> index 805e1fc74d..139f049ffc 100644
> --- a/accel/tcg/tb-stats.c
> +++ b/accel/tcg/tb-stats.c
> @@ -267,6 +267,25 @@ void do_hmp_tbstats_safe(CPUState *cpu, run_on_cpu_data icmd)
>       g_free(cmdinfo);
>   }
>   
> +/*
> + * We have to reset the tbs array on a tb_flush as those
> + * TranslationBlocks no longer exist and we no loner know if the
> + * current mapping is still valid.
> + */

The "and we no longer..." part is irrelevant: that's what phys_pc checks.
But the TranslationBlocks no longer exist, so that is sufficient.


r~
Re: [PATCH v14 07/10] tb-stats: reset the tracked TBs on a tb_flush
Posted by Wu, Fei 2 years, 8 months ago
On 6/1/2023 9:30 AM, Richard Henderson wrote:
> On 5/30/23 01:35, Fei Wu wrote:
>> diff --git a/accel/tcg/tb-stats.c b/accel/tcg/tb-stats.c
>> index 805e1fc74d..139f049ffc 100644
>> --- a/accel/tcg/tb-stats.c
>> +++ b/accel/tcg/tb-stats.c
>> @@ -267,6 +267,25 @@ void do_hmp_tbstats_safe(CPUState *cpu,
>> run_on_cpu_data icmd)
>>       g_free(cmdinfo);
>>   }
>>   +/*
>> + * We have to reset the tbs array on a tb_flush as those
>> + * TranslationBlocks no longer exist and we no loner know if the
>> + * current mapping is still valid.
>> + */
> 
> The "and we no longer..." part is irrelevant: that's what phys_pc checks.
> But the TranslationBlocks no longer exist, so that is sufficient.
> 
Will remove this part from comment.

Thanks,
Fei.

> 
> r~