Message-ID: <Z6qDXAjJBISQmvGA@google.com>
Date: Mon, 10 Feb 2025 14:53:16 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Dmitry Vyukov <dvyukov@...gle.com>
Cc: James Clark <james.clark@...aro.org>, linux-perf-users@...r.kernel.org,
	linux-kernel@...r.kernel.org, irogers@...gle.com
Subject: Re: [PATCH] perf report: Fix input reload/switch functionality

Hello,

On Mon, Jan 13, 2025 at 04:39:57PM +0100, Dmitry Vyukov wrote:
> On Mon, 13 Jan 2025 at 16:19, James Clark <james.clark@...aro.org> wrote:
> >
> > On 10/01/2025 12:19 pm, Dmitry Vyukov wrote:
> > > On Thu, 9 Jan 2025 at 15:50, James Clark <james.clark@...aro.org> wrote:
> > >> On 08/01/2025 3:35 pm, Dmitry Vyukov wrote:
> > >>> On Wed, 8 Jan 2025 at 16:23, James Clark <james.clark@...aro.org> wrote:
> > >>>>
> > >>>> On 08/01/2025 6:36 am, Dmitry Vyukov wrote:
> > >>>>> Currently the code checks that there is no "ipc" in the sort order
> > >>>>> and add an ipc string. This will always error out on the second pass
> > >>>>> after input reload/switch, since the sort order already contains "ipc".
> > >>>>> Do the ipc check/fixup only on the first pass.
> > >>>>>
> > >>>>
> > >>>> Hi Dmitry,
> > >>>>
> > >>>> A reproducer with before and after behavior might be helpful for the review.
> > >>>>
> > >>>> It might be unrelated to your change here, but the input switch thing
> > >>>> didn't seem to do anything for me. If I record two files, open the first
> > >>>> one, press S and select the second file nothing seems to change. I
> > >>>> assumed it would show the other file but nothing changes?
> > >>>>
> > >>>>     $ perf record -- true
> > >>>>     $ perf record -o 2.data -- ls
> > >>>>     $ perf report
> > >>>>     S key
> > >>>>     load 2.data
> > >>>
> > >>> Yes, sure. This needs "--sort symbol":
> > >>>
> > >>> perf report --sort symbol
> > >>>
> > >>> then press 's', select file, and it terminates.
> > >>>
> > >>> Affects fewer cases then I initially assumed, since I happened to run
> > >>> with "--sort symbol" when I discovered it.
> > >>>
> > >>>
> > >>
> > >> Ah yeah I can reproduce the bug with --sort, which is fixed by the
> > >> patch. But 's' doesn't actually reload a new histogram for me, I just
> > >> get the original file again. Maybe I'm misunderstanding what 's' is
> > >> supposed to do?
> > >
> > > As far as I understand, 's' allows you to select and load a new
> > > profile w/o existing (fwiw, if you select a different file). If the
> >
> > By 'without existing' do you mean running perf report with no perf.data
> > file? It won't run like that it just says "failed to open perf.data"
> >
> > > file has changed, I guess you can also reload it and see the changes.
> > >
> >
> > I wasn't able to make that work. How do you reload? If I change screen
> > between different events I still get the old file and I didn't see
> > 'reload' mentioned in the keybinding popup.
> >
> > I suppose my point is maybe it's not worth fixing the sort order bug if
> > we can just remove input file switching. The behavior doesn't seem any
> > different to v5.8 in case its a regression. Maybe I'm just doing
> > something wrong though.
> 
> 
> Humm... indeed, it does not seem to actually reload anything (even
> though the "Processing events..." progress bar progresses again).
> 
> I tried to use the reloading feature while implementing:
> 
> perf report: Add wall-clock and parallelism profiling
> https://lore.kernel.org/linux-perf-users/20250113134022.2545894-1-dvyukov@google.com/
> 
> to change some of the flags that can't be changed otherwise, but then
> dropped this idea since it was too messy, but decided to fix the
> failure I discovered.
> 
> So, yes, I guess we can drop this patch (I don't need it anymore).

Sorry for the long delay but I think we need this fix anyway.  I'll take
a look at the reload issue later.

Thanks,
Namhyung

Powered by blists - more mailing lists