Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] perf data: Add JSON export
To:     Jiri Olsa <jolsa@redhat.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Ian Rogers <irogers@google.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Stephane Eranian <eranian@google.com>,
        Tan Xiaojun <tanxiaojun@huawei.com>,
        Jin Yao <yao.jin@linux.intel.com>, linux-kernel@vger.kernel.org
References: <c4d0b0f1-79f4-f08d-3d7e-00046120f845@codeweavers.com>
 <YGXIDGfCLVtFuxgT@krava>
From:   Nicholas Fraser <nfraser@codeweavers.com>
Message-ID: <0831d8ee-12d5-dd28-dde4-a46e4781a76f@codeweavers.com>
Date:   Tue, 6 Apr 2021 05:31:51 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <YGXIDGfCLVtFuxgT@krava>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Jiri,

Thanks again for the review. I've fixed the issues you mentioned. Some notes
below:


On 2021-04-01 8:15 a.m., Jiri Olsa wrote:
> I recall you did not add support for walltime clock,
> don't you need it to sync with other events?

Not necessarily. As long as the perf recording and the GPU trace are made from
the same clock, the events should line up; the real time doesn't matter. I've
added it anyway though since it's good to have. It now outputs "clockid",
"clock-time" and "real-time" in the headers if clock info was included in the
recording.


On 2021-04-01 9:18 a.m., Jiri Olsa wrote:
> also I will not push hard for test, becase we don't have any for CTF ;-)
> but if you could think of any, that'd be great

I'm not sure what kind of tests would be useful. We could include a small
perf.data file and its JSON output and verify that they match. This is probably
pointless because almost any change to the JSON code would be expected to
change the output so we'd just be resetting the test on each change.


> I was wondering how to make this \t mess more readable,
> how about you define function like output_json:
> 
> 	output_json(FILE, level, field, format, ...);
> 
> and use it:
> 
> 	output_json(c->out, 3, "data-offset", "PRIu64", header->data_offset);
> 	output_json(c->out, 3, "data-size", "PRIu64", header->data_size);
> 	output_json(c->out, 3, "feat-offset", PRIu64, header->feat_offset);
> 
> similar way as we do for pr_debug -> eprintf

I've cleaned up the output with some additional helper functions. This is
essentially turning into its own mini JSON encoder which I was hoping to avoid
but I suppose it's necessary to make the code maintainable.


I'll be sending a new patch shortly.

Nick