[PATCH 5/8] arm64:perf: L3 cache(LLC) event counting in perf
Anurup M
anurupvasu at gmail.com
Tue Aug 2 17:33:07 PDT 2016
On Tuesday 28 June 2016 04:28 PM, Mark Rutland wrote:
> On Tue, Jun 28, 2016 at 05:50:26AM -0400, Anurup M wrote:
>> 1. Add support to count L3 cache(LLC) hardware events.
>> 2. Add events as RAW events.
>> The events to count can be selected as RAW event
>> referring to the hardware user manual.
>> e.g.: To input as RAW events the RAW event code is
>> -e r124f301
>> The RAW event encoding followed is
>> <socket ID(2 bit)><SCCL ID(4 bit)><ModuleID(4 bit)>
>> <Bank(4 bit)><event code(12 bit)>
>> So 0x1 is SocketID, 0x2 is SCCL ID, 0x4 is the LLC
>> hardware module ID, 0xf is for sum of all LLC banks,
>> 0x301 is the event code for LLC_READ_ALLOCATE.
> If you have a special way of encoding events, please add documentation
> for this, as we do for CCN, and as is being done for the X-Gene SoC PMU
> driver [1].
>
> The commit message is not an appropriate location to document a
> user-facing ABI which the user needs to know about.
>
> For example, I have no idea what a "SCCL ID" is, how the LLC HW module
> ID corresponds topologically, what a djtag unit is, nor how a djtag unit
> relates to other units in the system.
>
> Given that, and the lack of information regarding the djtag unit(s),
> I've skimmed over the rest of this patch, ignoring the portions I canot
> review due to a lack of appropriate information.
OK. I shall add the event details in Documentation/perf/hip05-pmu.txt
>
>> +u64 hisi_llc_event_update(struct perf_event *event,
>> + struct hw_perf_event *hwc, int idx)
>> +{
>> + struct device_node *djtag_node;
>> + struct hisi_pmu *pllc_pmu = to_hisi_pmu(event->pmu);
>> + struct hisi_hwmod_unit *punit;
>> + struct hisi_llc_data *llc_hwmod_data;
>> + u64 delta, prev_raw_count, new_raw_count = 0;
>> + int cfg_en;
>> + u32 raw_event_code = hwc->config_base;
>> + u32 scclID = (raw_event_code & HISI_SCCL_MASK) >> 20;
>> + u32 llc_idx = scclID - 1;
>> + int i;
>> +
>> + if (!scclID || (scclID >= HISI_SCCL_MASK)) {
>> + pr_err("Invalid SCCL=%d in event code!\n", scclID);
>> + return 0;
>> + }
>> +
>> + if (!hisi_llc_counter_valid(idx)) {
>> + pr_err("Unsupported event index:%d!\n", idx);
>> + return 0;
>> + }
>> +
>> + punit = &pllc_pmu->hwmod_pmu_unit[llc_idx];
>> + llc_hwmod_data = punit->hwmod_data;
>> +
>> + /* Check if the LLC data is initialized for this SCCL */
>> + if (!llc_hwmod_data->djtag_node) {
>> + pr_err("SCCL=%d not initialized!\n", scclID);
>> + return 0;
>> + }
>> +
>> + /* Find the djtag device node of the SCCL */
>> + djtag_node = llc_hwmod_data->djtag_node;
> As above, I cannot follow this due to a lack of understanding of the HW.
> You will need to add documentation such that this can be understood.
OK. shall add in the documentation.
>
> [...]
>
>> + /* Increment the active_events for the first event counting */
>> + if (!atomic_inc_not_zero(active_events)) {
>> + mutex_lock(&punit->reserve_mutex);
>> + if (atomic_read(active_events) == 0)
>> + atomic_inc(active_events);
>> + mutex_unlock(&punit->reserve_mutex);
>> + }
> If you have no special work to do to activate the PMU HW, this reference
> counting is pointless. Please get rid of it entirely.
>
> [...]
OK. I shall remove them.
>
>> +int hisi_llc_get_event_idx(struct hisi_hwmod_unit *punit)
>> +{
>> + struct hisi_llc_data *llc_hwmod_data = punit->hwmod_data;
>> + int event_idx;
>> +
>> + event_idx =
>> + find_first_zero_bit(
>> + llc_hwmod_data->hisi_llc_event_used_mask,
>> + HISI_MAX_CFG_LLC_CNTR);
>> +
>> + if (event_idx == HISI_MAX_CFG_LLC_CNTR)
>> + return -EAGAIN;
>> +
>> + __set_bit(event_idx,
>> + llc_hwmod_data->hisi_llc_event_used_mask);
> For reference, what mutual exclusion mechanism protects the used_mask?
>
> [...]
I shall check it. Thanks.
>> +void hisi_llc_pmu_init(struct platform_device *pdev,
>> + struct hisi_pmu *pllc_pmu)
>> +{
>> + pllc_pmu->pmu_type = SCCL_SPECIFIC;
>> + pllc_pmu->name = "HISI_L3C";
> Use lowercase. I suspect "hip05_l3c" is likely a better name, as
> Hisilcon may produce a new, different L3C controller, and you already
> use "hisilicon,hip05-llc" as the compatible string.
>
> [...]
OK. I shall use hip05_l3c for the pmu name,
>> + pllc_pmu->pmu = (struct pmu) {
>> + .name = "HISI-L3C-PMU",
>> + /* .task_ctx_nr = perf_invalid_context, */
> Why is this commented out? You *must* use perf_invalid_context.
>
>> + .pmu_enable = hisi_uncore_pmu_enable,
>> + .pmu_disable = hisi_uncore_pmu_disable,
>> + .event_init = hisi_uncore_pmu_event_init,
>> + .add = hisi_uncore_pmu_add,
>> + .del = hisi_uncore_pmu_del,
>> + .start = hisi_uncore_pmu_start,
>> + .stop = hisi_uncore_pmu_stop,
>> + .read = hisi_uncore_pmu_read,
>> + };
> [...]
>
>> +MODULE_DESCRIPTION("HiSilicon ARMv8 LLC PMU driver");
> s/ARMv8/HIP05/, the SoC matters far more than the architecture it
> happens to use.
Ok.
Thanks,
Anurup
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438607.html
More information about the linux-arm-kernel
mailing list