Hi Mark,
On Wed, Nov 6, 2019 at 3:28 PM Ganapatrao Kulkarni <gklkml16@xxxxxxxxx> wrote:
>
> Hi Peter, Mark,
>
> On Wed, Nov 6, 2019 at 3:28 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> >
> > On Wed, Nov 06, 2019 at 01:01:40AM +0000, Ganapatrao Prabhakerrao Kulkarni wrote:
> > > When PMUs are registered, perf core enables event multiplexing
> > > support by default. There is no provision for PMUs to disable
> > > event multiplexing, if PMUs want to disable due to unavoidable
> > > circumstances like hardware errata etc.
> > >
> > > Adding PMU capability flag PERF_PMU_CAP_NO_MUX_EVENTS and support
> > > to allow PMUs to explicitly disable event multiplexing.
> >
> > Even without multiplexing, this PMU activity can happen when switching
> > tasks, or when creating/destroying events, so as-is I don't think this
> > makes much sense.
> >
> > If there's an erratum whereby heavy access to the PMU can lockup the
> > core, and it's possible to workaround that by minimzing accesses, that
> > should be done in the back-end PMU driver.
>
> As said in errata, If there are heavy access to memory like stream
> application running and along with that if PMU control registers are
> also accessed frequently, then CPU lockup is seen.
>
> I ran perf stat with 4 events of thuderx2 PMU as well as with 6 events
> for stream application.
> For 4 events run, there is no event multiplexing, where as for 6
> events run the events are multiplexed.
>
> For 4 event run:
> No of times pmu->add is called: 10
> No of times pmu->del is called: 10
> No of times pmu->read is called: 310
>
> For 6 events run:
> No of times pmu->add is called: 5216
> No of times pmu->del is called: 5216
> No of times pmu->read is called: 5216
>
> Issue happens when the add and del are called too many times as seen
> with 6 event case.
> The PMU hardware control registers are programmed when add and del
> functions are called.
> For pmu->read no issues since no h/w issue with the data path.
>
> This is UNCORE driver, not sure context switch has any influence on this?
> Please suggest me, how can we fix this in back-end PMU driver without
> any perf core help?
>
> >
> > Either way, this minimzes the utility of the PMU.
> >
> > Thanks,
> > Mark.
> >
> > >
> > > Signed-off-by: Ganapatrao Prabhakerrao Kulkarni <gkulkarni@xxxxxxxxxxx>
> > > ---
> > > include/linux/perf_event.h | 1 +
> > > kernel/events/core.c | 8 ++++++++
> > > 2 files changed, 9 insertions(+)
> > >
> > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> > > index 61448c19a132..9e18d841daf7 100644
> > > --- a/include/linux/perf_event.h
> > > +++ b/include/linux/perf_event.h
> > > @@ -247,6 +247,7 @@ struct perf_event;
> > > #define PERF_PMU_CAP_HETEROGENEOUS_CPUS 0x40
> > > #define PERF_PMU_CAP_NO_EXCLUDE 0x80
> > > #define PERF_PMU_CAP_AUX_OUTPUT 0x100
> > > +#define PERF_PMU_CAP_NO_MUX_EVENTS 0x200
> > >
> > > /**
> > > * struct pmu - generic performance monitoring unit
> > > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > > index 4655adbbae10..65452784f81c 100644
> > > --- a/kernel/events/core.c
> > > +++ b/kernel/events/core.c
> > > @@ -1092,6 +1092,10 @@ static void __perf_mux_hrtimer_init(struct perf_cpu_context *cpuctx, int cpu)
> > > if (pmu->task_ctx_nr == perf_sw_context)
> > > return;
> > >
> > > + /* No PMU support */
> > > + if (pmu->capabilities & PERF_PMU_CAP_NO_MUX_EVENTS)
> > > + return 0;
> > > +
> > > /*
> > > * check default is sane, if not set then force to
> > > * default interval (1/tick)
> > > @@ -1117,6 +1121,10 @@ static int perf_mux_hrtimer_restart(struct perf_cpu_context *cpuctx)
> > > if (pmu->task_ctx_nr == perf_sw_context)
> > > return 0;
> > >
> > > + /* No PMU support */
> > > + if (pmu->capabilities & PERF_PMU_CAP_NO_MUX_EVENTS)
> > > + return 0;
> > > +
> > > raw_spin_lock_irqsave(&cpuctx->hrtimer_lock, flags);
> > > if (!cpuctx->hrtimer_active) {
> > > cpuctx->hrtimer_active = 1;
> > > --
> > > 2.17.1
> > >
>
> Thanks,
> Ganapat
Below diff does workaround without support of perf core.
Please review and let me know your thoughts?
root@SBR-26>perf>> git diff
diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
index 43d76c85da56..d5c90a93e96b 100644
--- a/drivers/perf/thunderx2_pmu.c
+++ b/drivers/perf/thunderx2_pmu.c
@@ -69,6 +69,7 @@ struct tx2_uncore_pmu {
int node;
int cpu;
u32 max_counters;
+ bool events_mux_disable;
u32 prorate_factor;
u32 max_events;
u64 hrtimer_interval;
@@ -442,6 +443,8 @@ static int tx2_uncore_event_init(struct perf_event *event)
if (!tx2_uncore_validate_event_group(event))
return -EINVAL;
+ /* reset flag */
+ tx2_pmu->events_mux_disable = false;
return 0;
}
@@ -490,10 +493,19 @@ static int tx2_uncore_event_add(struct
perf_event *event, int flags)
tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+ /* Erratum ThunderX2 erratum 221.
+ * Disable support for events multiplexing.
+ * Limiting the number of events to available hardware counters.
+ */
+ if (tx2_pmu->events_mux_disable)
+ return -EOPNOTSUPP;
+
/* Allocate a free counter */
hwc->idx = alloc_counter(tx2_pmu);
- if (hwc->idx < 0)
+ if (hwc->idx < 0) {
+ tx2_pmu->events_mux_disable = true;
return -EAGAIN;
+ }
tx2_pmu->events[hwc->idx] = event;
/* set counter control and data registers base address */
@@ -648,6 +660,7 @@ static struct tx2_uncore_pmu
*tx2_uncore_pmu_init_dev(struct device *dev,
tx2_pmu->dev = dev;
tx2_pmu->type = type;
tx2_pmu->base = base;
+ tx2_pmu->events_mux_disable = false;
tx2_pmu->node = dev_to_node(dev);
INIT_LIST_HEAD(&tx2_pmu->entry);
