[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qht: do not segfault when gathering stats from
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] qht: do not segfault when gathering stats from an uninitialized qht |
Date: |
Sat, 23 Jul 2016 12:54:51 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
On 23/07/2016 12:01, Peter Maydell wrote:
> On 22 July 2016 at 17:36, Emilio G. Cota <address@hidden> wrote:
>> So far, QHT functions assume that the passed qht has previously been
>> initialized--otherwise they segfault.
>>
>> This patch makes an exception for qht_statistics_init, with the goal
>> of simplifying calling code. For instance, qht_statistics_init is
>> called from the 'info jit' dump, and given that under KVM the TB qht
>> is never initialized, we get a segfault. Thus, instead of complicating
>> the 'info jit' code with additional checks, let's allow passing an
>> uninitialized qht to qht_statistics_init.
>>
>> While at it, add a test for this to test-qht.
>>
>> Before the patch (for $ qemu -enable-kvm [...]):
>> (qemu) info jit
>> [...]
>> direct jump count 0 (0%) (2 jumps=0 0%)
>> Program received signal SIGSEGV, Segmentation fault.
>>
>> After the patch:
>> (qemu) info jit
>> [...]
>> direct jump count 0 (0%) (2 jumps=0 0%)
>> TB hash buckets 0/0 (-nan% head buckets used)
>> TB hash occupancy nan% avg chain occ. Histogram: (null)
>> TB hash avg chain nan buckets. Histogram: (null)
>
> This looks like we're passing NULL pointers to
> printf %s specifiers. This is undefined behaviour at least
> for POSIX printf, and I can't see anything in the glib
> printf-alike function documentation that gives an extra
> guarantee for this, so it's probably a bad idea.
>
> Printing 'nan' also looks a bit odd, though it's not UB.
Let's move everything to a new function, so that it's easy to add a
check at the top:
diff --git a/translate-all.c b/translate-all.c
index 0d47c1c..efeba29 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1663,15 +1663,50 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong
addr)
TB_JMP_PAGE_SIZE * sizeof(TranslationBlock *));
}
+static void print_qht_statistics(FILE *f, fprintf_function cpu_fprintf,
+ struct qht_stats hst)
+{
+ uint32_t hgram_opts;
+ size_t hgram_bins;
+ char *hgram;
+
+ if (!hst.head_buckets) {
+ return;
+ }
+ cpu_fprintf(f, "TB hash buckets %zu/%zu (%0.2f%% head buckets used)\n",
+ hst.used_head_buckets, hst.head_buckets,
+ (double)hst.used_head_buckets / hst.head_buckets * 100);
+
+ hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
+ hgram_opts |= QDIST_PR_100X | QDIST_PR_PERCENT;
+ if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
+ hgram_opts |= QDIST_PR_NODECIMAL;
+ }
+ hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
+ cpu_fprintf(f, "TB hash occupancy %0.2f%% avg chain occ. Histogram:
%s\n",
+ qdist_avg(&hst.occupancy) * 100, hgram);
+ g_free(hgram);
+
+ hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
+ hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
+ if (hgram_bins > 10) {
+ hgram_bins = 10;
+ } else {
+ hgram_bins = 0;
+ hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
+ }
+ hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
+ cpu_fprintf(f, "TB hash avg chain %0.3f buckets. Histogram: %s\n",
+ qdist_avg(&hst.chain), hgram);
+ g_free(hgram);
+}
+
void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
{
int i, target_code_size, max_target_code_size;
int direct_jmp_count, direct_jmp2_count, cross_page;
TranslationBlock *tb;
struct qht_stats hst;
- uint32_t hgram_opts;
- size_t hgram_bins;
- char *hgram;
target_code_size = 0;
max_target_code_size = 0;
@@ -1724,34 +1759,7 @@ void dump_exec_info(FILE *f, fprintf_function
cpu_fprintf)
tcg_ctx.tb_ctx.nb_tbs : 0);
qht_statistics_init(&tcg_ctx.tb_ctx.htable, &hst);
-
- cpu_fprintf(f, "TB hash buckets %zu/%zu (%0.2f%% head buckets used)\n",
- hst.used_head_buckets, hst.head_buckets,
- (double)hst.used_head_buckets / hst.head_buckets * 100);
-
- hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
- hgram_opts |= QDIST_PR_100X | QDIST_PR_PERCENT;
- if (qdist_xmax(&hst.occupancy) - qdist_xmin(&hst.occupancy) == 1) {
- hgram_opts |= QDIST_PR_NODECIMAL;
- }
- hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
- cpu_fprintf(f, "TB hash occupancy %0.2f%% avg chain occ. Histogram:
%s\n",
- qdist_avg(&hst.occupancy) * 100, hgram);
- g_free(hgram);
-
- hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
- hgram_bins = qdist_xmax(&hst.chain) - qdist_xmin(&hst.chain);
- if (hgram_bins > 10) {
- hgram_bins = 10;
- } else {
- hgram_bins = 0;
- hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
- }
- hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
- cpu_fprintf(f, "TB hash avg chain %0.3f buckets. Histogram: %s\n",
- qdist_avg(&hst.chain), hgram);
- g_free(hgram);
-
+ print_qht_statistics(f, cpu_fprintf, hst);
qht_statistics_destroy(&hst);
cpu_fprintf(f, "\nStatistics:\n");