Messages in this thread |  | | Date | Mon, 10 Feb 2025 14:53:16 -0800 | | From | Namhyung Kim <> | | 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@linaro.org> wrote: > > > > On 10/01/2025 12:19 pm, Dmitry Vyukov wrote: > > > On Thu, 9 Jan 2025 at 15:50, James Clark <james.clark@linaro.org> wrote: > > >> On 08/01/2025 3:35 pm, Dmitry Vyukov wrote: > > >>> On Wed, 8 Jan 2025 at 16:23, James Clark <james.clark@linaro.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
|  |