From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4C2ABC77B73 for ; Tue, 6 Jun 2023 11:17:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232782AbjFFLRE (ORCPT ); Tue, 6 Jun 2023 07:17:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48782 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229803AbjFFLRD (ORCPT ); Tue, 6 Jun 2023 07:17:03 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A338DB1 for ; Tue, 6 Jun 2023 04:16:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1686050180; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=N0sKgVlueJOT01kIpStlVpZoquMJIjLQCB74eD8luzc=; b=LhYiwAJRqTbek1HPX0RU3MMGf1+yJNWKgy0zdfSHPKdnJHuSS+OprFKtA2UboOrrNuuQad 0z9tYJxoRtf15cQ1JIQeu8pcrJm3UhHi2yFuJl1/BR54i9l++4+SNnCTq1JO9dVAiDZH15 eFywLzwnXilOyswwLNQ4rC+aWE0SHog= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-301-3Gy0ke__MGOVrOSUblgnHQ-1; Tue, 06 Jun 2023 07:16:19 -0400 X-MC-Unique: 3Gy0ke__MGOVrOSUblgnHQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 51C8B3C14107; Tue, 6 Jun 2023 11:16:19 +0000 (UTC) Received: from Diego (unknown [10.39.208.23]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5EBCAC1603B; Tue, 6 Jun 2023 11:16:17 +0000 (UTC) Date: Tue, 6 Jun 2023 13:16:14 +0200 (CEST) From: Michael Petlan X-X-Sender: Michael@Diego To: acme@kernel.org cc: linux-perf-users@vger.kernel.org, acme@redhat.com, qzhao@redhat.com, cjense@google.com, irogers@google.com Subject: Re: [PATCH 1/2] perf stat: Fix JSON metric printout for multiple metrics per line In-Reply-To: Message-ID: References: <20230120134039.17788-1-mpetlan@redhat.com> <20230120134039.17788-2-mpetlan@redhat.com> User-Agent: Alpine 2.20 (LRH 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 Precedence: bulk List-ID: X-Mailing-List: linux-perf-users@vger.kernel.org Gently ping... On Mon, 22 May 2023, Michael Petlan wrote: > Hello Ian and Arnaldo, > > I've recently got back to the JSON test failures. In January, I post two > patches within a patchset. I see that Ian has updated the testcase code, > so the second patch is probably no more needed. However I don't see this > one applied upstream yet, although it seemed on the list, it had got some > acks. Has it been forgotten somehow? > > To remind, since the JSON printer engine wasn't ready to print multiple > metrics per line, the JSON output got screwed up in that situation. The > situation happened on some SPR CPUs, where Top-Down metrics are supported. > > The patch below fixes the printer to handle this situation correctly. > > I remember someone saying that the whole printer engine needs to be > refactored some day, but I'd think that before that happens, it'd be > nice to have it fixed... > > What do you think? > > Regards, > Michael > > On Fri, 20 Jan 2023, Michael Petlan wrote: > > JSON printing engine used to always print metric (even when there should > > be none and other engines (std, csv) would not print it) and the metric > > used to always close the line by printing a closing curly bracket. > > > > This caused invalid JSON output being generated for top-down metrics, > > when multiple metrics are printed on a single line, so the broken output > > might have looked like: > > > > ... "metric-value" : 15.564203, "metric-unit" : \ > > "Memory Bound"}"metric-value" : 14.007787, "metric-unit" : "Core Bound"} > > > > To fix it, print always the separating comma BEFORE the key:value pairs > > and close the line outside of the JSON metric printing routine. > > > > Fixes: df936cadfb58 ("perf stat: Add JSON output option") > > > > Signed-off-by: Michael Petlan > > --- > > tools/perf/util/stat-display.c | 28 ++++++++++++++++------------ > > 1 file changed, 16 insertions(+), 12 deletions(-) > > > > diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c > > index 8bd8b0142630..8f80f6b566d0 100644 > > --- a/tools/perf/util/stat-display.c > > +++ b/tools/perf/util/stat-display.c > > @@ -86,7 +86,7 @@ static void print_running_json(struct perf_stat_config *config, u64 run, u64 ena > > > > if (run != ena) > > enabled_percent = 100 * run / ena; > > - fprintf(config->output, "\"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f, ", > > + fprintf(config->output, ", \"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f", > > run, enabled_percent); > > } > > > > @@ -121,7 +121,7 @@ static void print_noise_pct_csv(struct perf_stat_config *config, > > static void print_noise_pct_json(struct perf_stat_config *config, > > double pct) > > { > > - fprintf(config->output, "\"variance\" : %.2f, ", pct); > > + fprintf(config->output, ", \"variance\" : %.2f", pct); > > } > > > > static void print_noise_pct(struct perf_stat_config *config, > > @@ -165,7 +165,7 @@ static void print_cgroup_csv(struct perf_stat_config *config, const char *cgrp_n > > > > static void print_cgroup_json(struct perf_stat_config *config, const char *cgrp_name) > > { > > - fprintf(config->output, "\"cgroup\" : \"%s\", ", cgrp_name); > > + fprintf(config->output, ", \"cgroup\" : \"%s\"", cgrp_name); > > } > > > > static void print_cgroup(struct perf_stat_config *config, struct cgroup *cgrp) > > @@ -431,10 +431,11 @@ static void print_metric_json(struct perf_stat_config *config __maybe_unused, > > struct outstate *os = ctx; > > FILE *out = os->fh; > > > > - fprintf(out, "\"metric-value\" : %f, ", val); > > - fprintf(out, "\"metric-unit\" : \"%s\"", unit); > > - if (!config->metric_only) > > - fprintf(out, "}"); > > + if (unit == NULL || fmt == NULL) > > + return; > > + > > + fprintf(out, ", \"metric-value\" : %f", val); > > + fprintf(out, ", \"metric-unit\" : \"%s\"", unit); > > } > > > > static void new_line_json(struct perf_stat_config *config, void *ctx) > > @@ -623,14 +624,14 @@ static void print_counter_value_json(struct perf_stat_config *config, > > const char *bad_count = evsel->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED; > > > > if (ok) > > - fprintf(output, "\"counter-value\" : \"%f\", ", avg); > > + fprintf(output, "\"counter-value\" : \"%f\"", avg); > > else > > - fprintf(output, "\"counter-value\" : \"%s\", ", bad_count); > > + fprintf(output, "\"counter-value\" : \"%s\"", bad_count); > > > > if (evsel->unit) > > - fprintf(output, "\"unit\" : \"%s\", ", evsel->unit); > > + fprintf(output, ", \"unit\" : \"%s\"", evsel->unit); > > > > - fprintf(output, "\"event\" : \"%s\", ", evsel__name(evsel)); > > + fprintf(output, ", \"event\" : \"%s\"", evsel__name(evsel)); > > } > > > > static void print_counter_value(struct perf_stat_config *config, > > @@ -835,8 +836,11 @@ static void print_counter_aggrdata(struct perf_stat_config *config, > > > > printout(config, os, uval, run, ena, avg, s); > > > > - if (!metric_only) > > + if (!metric_only) { > > + if (config->json_output) > > + fputc('}', output); > > fputc('\n', output); > > + } > > } > > > > static void print_metric_begin(struct perf_stat_config *config, > > -- > > 2.18.4 > > > > > >