Re: [PATCH v5 0/2] perf: riscv: Preliminary Perf Event Support on RISC-V

[Atish Patra]

On 4/24/18 8:19 PM, Alan Kao wrote:
> Hi Atish, Palmer,
>
> On Tue, Apr 24, 2018 at 06:15:49PM -0700, Atish Patra wrote:
> > On 4/24/18 5:29 PM, Palmer Dabbelt wrote:
> > > On Tue, 24 Apr 2018 15:16:16 PDT (-0700), atish.patra@xxxxxxx wrote:
> > > > On 4/24/18 12:44 PM, Palmer Dabbelt wrote:
> > > > > On Tue, 24 Apr 2018 12:27:26 PDT (-0700), atish.patra@xxxxxxx wrote:
> > > > > > On 4/24/18 11:07 AM, Atish Patra wrote:
> > > > > > > On 4/19/18 4:28 PM, Alan Kao wrote:
> > > > > > > However, I got an rcu-stall for the test "47: Event times".
> > > > > > > # ./perf test -v 47
> > > > > > Got it working. The test tries to attach the event to CPU0 which doesn't
> > > > > > exist in HighFive Unleashed. Changing it to cpu1 works.
> > > > > >
> > > > > > diff --git a/tools/perf/tests/event-times.c b/tools/perf/tests/event-times.c
> > > > > > index 1a2686f..eb11632f 100644
> > > > > > --- a/tools/perf/tests/event-times.c
> > > > > > +++ b/tools/perf/tests/event-times.c
> > > > > > @@ -113,9 +113,9 @@ static int attach__cpu_disabled(struct perf_evlist
> > > > > > *evlist)
> > > > > >            struct cpu_map *cpus;
> > > > > >            int err;
> > > > > >
> > > > > > -       pr_debug("attaching to CPU 0 as enabled\n");
> > > > > > +       pr_debug("attaching to CPU 1 as disabled\n");
> > > > > >
> > > > > > -       cpus = cpu_map__new("0");
> > > > > > +       cpus = cpu_map__new("1");
> > > > > >            if (cpus == NULL) {
> > > > > >                    pr_debug("failed to call cpu_map__new\n");
> > > > > >                    return -1;
> > > > > > @@ -142,9 +142,9 @@ static int attach__cpu_enabled(struct perf_evlist
> > > > > > *evlist)
> > > > > >            struct cpu_map *cpus;
> > > > > >            int err;
> > > > > >
> > > > > > -       pr_debug("attaching to CPU 0 as enabled\n");
> > > > > > +       pr_debug("attaching to CPU 1 as enabled\n");
> > > > > >
> > > > > > -       cpus = cpu_map__new("0");
> > > > > > +       cpus = cpu_map__new("1");
> > > > > >            if (cpus == NULL) {
> > > > > >                    pr_debug("failed to call cpu_map__new\n");
> > > > > >                    return -1;
> > > > > >
> > > > > >
> > > > > > Palmer,
> > > > > > Would it be better to officially document it somewhere that CPU0 doesn't
> > > > > > exist in the HighFive Unleashed board ?
> > > > > > I fear that there will be other standard tests/code path that may fail
> > > > > > because of inherent assumption of cpu0 presence.
> > > > >
> > > > > I think the best way to fix this is to just have BBL (or whatever the
> > > > > bootloader is) renumber the CPUs so they're contiguous and begin with 0.
> > > >
> > > > Do you mean BBL will update the device tree that kernel eventually parse
> > > > and set the hart id?
> > > > Sounds good to me unless it acts as a big hack in future boot loaders.
> > >
> > > Right now the machine-mode and supervisor-mode hart IDs are logically separate:
> > > the bootloader just provides the hart ID as a register argument when starting
> > > the kernel.
> >
> > Yes.
> >
> >    BBL already needs to enumerate the harts by looking through the
> > > device tree for various other reasons (at least to mask off the harts that
> > > Linux doesn't support), so it's not that much effort to just maintain a mapping
> > >from supervisor-mode hart IDs to machine-mode hart IDs.
> > >
> >
> > But Linux also parses the device tree to get hart ID in
> > riscv_of_processor_hart(). This is used to setup the possible/present cpu
> > map in setup_smp().
> >
> > Thus, Linux also need to see a device tree with cpu0-3 instead of cpu1-4.
> > Otherwise, present cpu map will be incorrect. Isn't it ?
> >
> > > I have some patches floating around that do this, but appear to do it
> > > incorrectly enough that nothing boots so maybe I'm missing something that makes
> > > this complicated :).
> >
> > Just a wild guess: May be the because of the above reason ;)
>
> Thanks for the test and discussion.  It looks like am implementation issue from
> Unleash, so ... is there anything I should fix and provide a further patch?
Nope. Nothing for this issue.
The fix for this issue should be either in bbl or kernel as a separate 
patch.