From patchwork Sat Jul 23 10:54:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Paolo Bonzini X-Patchwork-Id: 651870 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3rxPZ60KcLz9t1V for ; Sat, 23 Jul 2016 20:55:40 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=RKtclXDF; dkim-atps=neutral Received: from localhost ([::1]:51345 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQuan-0008Rm-Kh for incoming@patchwork.ozlabs.org; Sat, 23 Jul 2016 06:55:37 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41527) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQuaB-00086Q-Rr for qemu-devel@nongnu.org; Sat, 23 Jul 2016 06:55:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bQua7-0004sI-EH for qemu-devel@nongnu.org; Sat, 23 Jul 2016 06:54:58 -0400 Received: from mail-wm0-x241.google.com ([2a00:1450:400c:c09::241]:35334) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bQua7-0004sB-3X for qemu-devel@nongnu.org; Sat, 23 Jul 2016 06:54:55 -0400 Received: by mail-wm0-x241.google.com with SMTP id i5so9352345wmg.2 for ; Sat, 23 Jul 2016 03:54:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=X9jmQSzS5HBKCmoR16We7D8KDUrA6zpKYnshHvtjRtQ=; b=RKtclXDFPfN0vG6O5fYsMfW8Qi/qOXCQeH8r+A9EWwD7P80NSM3S1TPc3w5uioK1gQ SgckOXLv9SpiGv+cnuY4ni2C3RqfYas1ZDIBUalA/E0gMWileXUiZ+QJVqnUx9qaatSZ wuNsbASs7sN5d9bjREkR6+mKppjX650Ua6rHmAjilIJYiP924FhPMgXHpanYFB/kQ9Sv 4fXbWnpr9s+U2ravTsze4feUdgWsAgMnYP9KBEj6x9Fv3G2l+KyZtTmldIbrErVe+40v EXeUD/LWOfx6jnpB5/TMQzLAA9NavQsHtyKLkFWwbzrVMwA/u/ilS8tqsAC+uxJQLPjJ SaAA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:subject:to:references:cc:from:message-id :date:user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=X9jmQSzS5HBKCmoR16We7D8KDUrA6zpKYnshHvtjRtQ=; b=loQ8tqBqJk07CTIfOlnjCO/VlrxrfVla9s/I8uqq84LmIoSAXbnlJIyZ5TzK/smy3c 0E5iUboECPDkYGzn5RQK9NQzX0MN+t+AMufJZnXtmbA3yeOMKEIwqnYK+KPUnmpj4xEU mbGQ0VOutgnT1HVIBYkLugqEoj/KWIG4P69bZYtNdRLM2O0USm2kFPZvF60rXj6NTGGk i/YDmSXnK3YHwgiS4E1S7TrwGCAKaf2FL33zBLKF6Z4wbVeyOPO62mtiDtXgf9fsqDZN H/+eqqLQRYHmf8Y2WaDiN1yDEqt46k8gI7SI1cnVgMAUHBhSN4bRptw8OtI2J7D6XSHO MGkQ== X-Gm-Message-State: ALyK8tIc4VgvkGFuBuVrGUKp2AujwoOuzzi27/JN09ClZl1qL8Rhk2ox09Oj/ZDCtaGFnw== X-Received: by 10.28.186.138 with SMTP id k132mr28795194wmf.65.1469271294321; Sat, 23 Jul 2016 03:54:54 -0700 (PDT) Received: from [192.168.10.150] (94-39-158-5.adsl-ull.clienti.tiscali.it. [94.39.158.5]) by smtp.googlemail.com with ESMTPSA id a3sm5332306wjm.30.2016.07.23.03.54.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 23 Jul 2016 03:54:52 -0700 (PDT) To: Peter Maydell , "Emilio G. Cota" References: <5791E191.4010404@cn.fujitsu.com> <1469205390-14369-1-git-send-email-cota@braap.org> From: Paolo Bonzini Message-ID: 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 MIME-Version: 1.0 In-Reply-To: X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:400c:c09::241 Subject: Re: [Qemu-devel] [PATCH] qht: do not segfault when gathering stats from an uninitialized qht X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Changlong Xie , QEMU Developers , Richard Henderson Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" On 23/07/2016 12:01, Peter Maydell wrote: > On 22 July 2016 at 17:36, Emilio G. Cota 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");