Received-SPF: pass (google.com: domain of linux-kernel+bounces-137481-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <8fb7e240-07af-4c7e-8eac-60731d837bd7@linux.intel.com>
Date: Tue, 9 Apr 2024 14:50:21 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] perf stat: Remove evlist__add_default_attrs use
 strings
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
 Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Adrian Hunter <adrian.hunter@intel.com>,
 James Clark <james.clark@arm.com>, Yang Jihong <yangjihong1@huawei.com>,
 Kaige Ye <ye@kaige.org>, Yicong Yang <yangyicong@hisilicon.com>,
 K Prateek Nayak <kprateek.nayak@amd.com>, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240406073804.1932415-1-irogers@google.com>
 <0d498870-b235-4860-9563-befcddf0ec3e@linux.intel.com>
 <CAP-5=fU=bEvzWw+62HxL=5kOSqQxaBYeDBomv8Fdu+R+fNv5sw@mail.gmail.com>
 <cd4b5229-b938-44ef-822a-68d29c463aa7@linux.intel.com>
 <CAP-5=fUNTiju7sv5R3P+=rUm-BzL_HmaEicdhCuBVPKooPHWeA@mail.gmail.com>
Content-Language: en-US
From: "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <CAP-5=fUNTiju7sv5R3P+=rUm-BzL_HmaEicdhCuBVPKooPHWeA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 2024-04-09 12:04 p.m., Ian Rogers wrote:
> On Tue, Apr 9, 2024 at 9:00 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>>
>>
>>
>> On 2024-04-09 11:20 a.m., Ian Rogers wrote:
>>>>> +             ret = parse_events(evlist,
>>>>> +                             "context-switches,"
>>>>> +                             "cpu-migrations,"
>>>>> +                             "page-faults,"
>>>>> +                             "instructions,"
>>>>> +                             "cycles,"
>>>> "cycles",
>>>> "instructions",
>>>>
>>>> It's better to keep the original order.
>>> So the original order was:
>>> "cycles,"
>>> "stalled-cycles-frontend,"
>>> "stalled-cycles-backend,"
>>> "instructions"
>>>
>>
>> Right. The stalled-* events are added between default_attrs0 and
>> default_attrs1.
>>
>>
>>> but many/most/all core PMUs don't provide the stalled-* events. At the
>>> programmer level instructions is the most fundamental thing, so having
>>> it last felt wrong hence moving it to be the first after the software
>>> events. My thought was, if we're going to reorder things then let's
>>> not do a half measure like:
>>> "cycles,"
>>> "instructions,"
>>> "stalled-cycles-frontend,"
>>> "stalled-cycles-backend"
>>>
>>> let's just put things into their best order. It is obviously easy to
>>> change but having this way wasn't an accident. There's obviously
>>> subjectivity about whether cycles is more fundamental than
>>> instructions, my thought is that you get taught instructions first and
>>> that these take some number of cycles to execute, hence thinking
>>> instructions should have some priority in the output over cycles -
>>> some people may not even know what cycles means, it is hard enough
>>> when you do given the variety of different clocks ????
>>>
>>
>> My concern is that there may be someone who still relies on the std
>> output of perf stat default. So the output format/order matters for
>> them. Their scripts probably be broken if the order is changed.
> 
> I think making everyone suffer for the case of a tool that may behave
> in this way doesn't make sense. The tool should transition to not care
> or to say the json output, or at least contribute a test. There is
> precedent for this attitude, the default metrics for topdown removed
> the event names in perf stat default output - no one screamed, and I
> expect that to be the case here.
> 

They did, but that happened after the change was merged. And there was
no test case for the output at that time.

I agree that if the order is important, there should be a test for it.
I've emailed the tool owners I know and see if the change impacts them.
But they are all out of office this week and should be back next week.
I will let you know regarding their feedback. If the order is important,
I will update the stat+std_output.sh.

Thanks,
Kan