| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 16 May 2022 19:11:17 +0800
From: "huangguangbin (A)" <huangguangbin2@...wei.com>
To: Will Deacon <will@...nel.org>
CC: <john.garry@...wei.com>, <mark.rutland@....com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linuxarm@...wei.com>,
<liuqi115@...wei.com>, <zhangshaokun@...ilicon.com>,
<f.fangjian@...wei.com>, <lipeng321@...wei.com>,
<shenjian15@...wei.com>
Subject: Re: [PATCH V6 2/2] drivers/perf: hisi: add driver for HNS3 PMU
On 2022/5/13 20:11, Will Deacon wrote:
> On Wed, Apr 27, 2022 at 08:10:00PM +0800, Guangbin Huang wrote:
>> HNS3(HiSilicon Network System 3) PMU is RCiEP device in HiSilicon SoC NIC,
>> supports collection of performance statistics such as bandwidth, latency,
>> packet rate and interrupt rate.
>>
>> NIC of each SICL has one PMU device for it. Driver registers each PMU
>> device to perf, and exports information of supported events, filter mode of
>> each event, bdf range, hardware clock frequency, identifier and so on via
>> sysfs.
>
> Curioous, but how similar is this device to the one which we already support
> in hisi_pcie_pmu.c? Can we reuse/share any of that existing code?
>
Hi Will,
Thanks very much for your comments!
HNS3 PMU is also a RCiEP device and also has two registers for each hardware event,
and I referred to code of hisi_pcie_pmu.c when writing this driver code, so this
driver is similar to hisi_pcie_pmu.c.
There is only a small amount of code in hisi_pcie_pmu.c can be reuse/share for HNS3
PMU driver, so I didn't reuse them because I think it is not very profitable. HNS3
PMU is different type of hardware and has it own iterative version in the future, so
I tend not to be coupled with code of hisi_pcie_pmu.c. And they have different prefix
too.
Do you think it is really necessary to reuse some code of hisi_pcie_pmu.c?
>> Each PMU device has its own registers of control, counters and interrupt,
>> and it supports 8 hardware events, each hardward event has its own
>> registers for configuration, counters and interrupt.
>>
>> Filter options contains:
>> event - select event
>> port - select physical port of nic
>> tc - select tc(must be used with port)
>> func - select PF/VF
>> queue - select queue of PF/VF(must be used with func)
>> intr - select interrupt number(must be used with func)
>> global - select all functions of IO DIE
>>
>> Signed-off-by: Guangbin Huang <huangguangbin2@...wei.com>
>> Reviewed-by: John Garry <john.garry@...wei.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/perf/hisilicon/Kconfig | 10 +
>> drivers/perf/hisilicon/Makefile | 1 +
>> drivers/perf/hisilicon/hns3_pmu.c | 1654 +++++++++++++++++++++++++++++
>> include/linux/cpuhotplug.h | 1 +
>> 5 files changed, 1672 insertions(+)
>> create mode 100644 drivers/perf/hisilicon/hns3_pmu.c
>
> Anyway, this mostly looks good to me, but I have a few small comments on the
> user ABI:
>
>> +#define HNS3_PMU_FILTER_ATTR(_name, _config, _start, _end) \
>> + static inline u64 hns3_pmu_get_##_name(struct perf_event *event) \
>> + { \
>> + return FIELD_GET(GENMASK_ULL(_end, _start), \
>> + event->attr._config); \
>> + }
>> +
>> +HNS3_PMU_FILTER_ATTR(event, config, 0, 16);
>> +HNS3_PMU_FILTER_ATTR(subevent, config, 0, 7);
>> +HNS3_PMU_FILTER_ATTR(event_type, config, 8, 15);
>> +HNS3_PMU_FILTER_ATTR(ext_counter_used, config, 16, 16);
>> +HNS3_PMU_FILTER_ATTR(real_event, config, 0, 15);
>> +HNS3_PMU_FILTER_ATTR(port, config1, 0, 3);
>> +HNS3_PMU_FILTER_ATTR(tc, config1, 4, 7);
>> +HNS3_PMU_FILTER_ATTR(bdf, config1, 8, 23);
>> +HNS3_PMU_FILTER_ATTR(queue, config1, 24, 39);
>> +HNS3_PMU_FILTER_ATTR(intr, config1, 40, 51);
>> +HNS3_PMU_FILTER_ATTR(global, config1, 52, 52);
>> +
>> +#define HNS3_BW_EVT_BYTE_NUM(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_BW_##_name##_BYTE_NUM, \
>> + HNS3_PMU_FILTER_BW_##_name})
>> +#define HNS3_BW_EVT_TIME(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_BW_##_name##_TIME, \
>> + HNS3_PMU_FILTER_BW_##_name})
>> +#define HNS3_PPS_EVT_PACKET_NUM(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_PPS_##_name##_PACKET_NUM, \
>> + HNS3_PMU_FILTER_PPS_##_name})
>> +#define HNS3_PPS_EVT_TIME(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_PPS_##_name##_TIME, \
>> + HNS3_PMU_FILTER_PPS_##_name})
>> +#define HNS3_DLY_EVT_TIME(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_DLY_##_name##_TIME, \
>> + HNS3_PMU_FILTER_DLY_##_name})
>> +#define HNS3_DLY_EVT_PACKET_NUM(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_DLY_##_name##_PACKET_NUM, \
>> + HNS3_PMU_FILTER_DLY_##_name})
>> +#define HNS3_INTR_EVT_INTR_NUM(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_PPS_##_name##_INTR_NUM, \
>> + HNS3_PMU_FILTER_INTR_##_name})
>> +#define HNS3_INTR_EVT_TIME(_name) (&(struct hns3_pmu_event_attr) {\
>> + HNS3_PMU_EVT_PPS_##_name##_TIME, \
>> + HNS3_PMU_FILTER_INTR_##_name})
>> +
>> +static ssize_t hns3_pmu_format_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct dev_ext_attribute *eattr;
>> +
>> + eattr = container_of(attr, struct dev_ext_attribute, attr);
>> +
>> + return sysfs_emit(buf, "%s\n", (char *)eattr->var);
>> +}
>> +
>> +static ssize_t hns3_pmu_event_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hns3_pmu_event_attr *event;
>> + struct dev_ext_attribute *eattr;
>> +
>> + eattr = container_of(attr, struct dev_ext_attribute, attr);
>> + event = eattr->var;
>> +
>> + return sysfs_emit(buf, "config=0x%05x\n", event->event);
>
> Are you sure you want the %05 here? The format of the number is
> user-visible, so you won't be able to change this in future. The other hisi
> PMU drivers use %lx or %llx.
>
Ok, I think you are right, I will modify it.
>> +}
>> +
>> +static ssize_t hns3_pmu_filter_mode_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct hns3_pmu_event_attr *event;
>> + struct dev_ext_attribute *eattr;
>> + int len;
>> +
>> + eattr = container_of(attr, struct dev_ext_attribute, attr);
>> + event = eattr->var;
>> +
>> + len = sysfs_emit_at(buf, 0, "filter mode supported: ");
>> + if (event->filter_support & HNS3_PMU_FILTER_SUPPORT_GLOBAL)
>> + len += sysfs_emit_at(buf, len, "global ");
>> + if (event->filter_support & HNS3_PMU_FILTER_SUPPORT_PORT)
>> + len += sysfs_emit_at(buf, len, "port ");
>> + if (event->filter_support & HNS3_PMU_FILTER_SUPPORT_PORT_TC)
>> + len += sysfs_emit_at(buf, len, "port-tc ");
>> + if (event->filter_support & HNS3_PMU_FILTER_SUPPORT_FUNC)
>> + len += sysfs_emit_at(buf, len, "func ");
>> + if (event->filter_support & HNS3_PMU_FILTER_SUPPORT_FUNC_QUEUE)
>> + len += sysfs_emit_at(buf, len, "func-queue ");
>> + if (event->filter_support & HNS3_PMU_FILTER_SUPPORT_FUNC_INTR)
>> + len += sysfs_emit_at(buf, len, "func-intr");
>
> It's a bit odd to have a trailing space for everything other than
> "func-intr" as it's not guaranteed to be the last entry (it might not be
> present).
>
Ok, I will modify it.
>> +
>> + len += sysfs_emit_at(buf, len, "\n");
>> +
>> + return len;
>> +}
>> +
>> +#define HNS3_PMU_ATTR(_name, _func, _config) \
>> + (&((struct dev_ext_attribute[]) { \
>> + { __ATTR(_name, 0444, _func, NULL), (void *)_config } \
>> + })[0].attr.attr)
>> +
>> +#define HNS3_PMU_FORMAT_ATTR(_name, _format) \
>> + HNS3_PMU_ATTR(_name, hns3_pmu_format_show, (void *)_format)
>> +#define HNS3_PMU_EVENT_ATTR(_name, _event) \
>> + HNS3_PMU_ATTR(_name, hns3_pmu_event_show, (void *)_event)
>> +#define HNS3_PMU_FLT_MODE_ATTR(_name, _event) \
>> + HNS3_PMU_ATTR(_name, hns3_pmu_filter_mode_show, (void *)_event)
>> +
>> +#define HNS3_PMU_BW_EVT_PAIR(_name, _macro) \
>> + HNS3_PMU_EVENT_ATTR(_name##_byte_num, HNS3_BW_EVT_BYTE_NUM(_macro)), \
>> + HNS3_PMU_EVENT_ATTR(_name##_time, HNS3_BW_EVT_TIME(_macro))
>> +#define HNS3_PMU_PPS_EVT_PAIR(_name, _macro) \
>> + HNS3_PMU_EVENT_ATTR(_name##_packet_num, HNS3_PPS_EVT_PACKET_NUM(_macro)), \
>> + HNS3_PMU_EVENT_ATTR(_name##_time, HNS3_PPS_EVT_TIME(_macro))
>> +#define HNS3_PMU_DLY_EVT_PAIR(_name, _macro) \
>> + HNS3_PMU_EVENT_ATTR(_name##_time, HNS3_DLY_EVT_TIME(_macro)), \
>> + HNS3_PMU_EVENT_ATTR(_name##_packet_num, HNS3_DLY_EVT_PACKET_NUM(_macro))
>> +#define HNS3_PMU_INTR_EVT_PAIR(_name, _macro) \
>> + HNS3_PMU_EVENT_ATTR(_name##_intr_num, HNS3_INTR_EVT_INTR_NUM(_macro)), \
>> + HNS3_PMU_EVENT_ATTR(_name##_time, HNS3_INTR_EVT_TIME(_macro))
>> +
>> +#define HNS3_PMU_BW_FLT_MODE_PAIR(_name, _macro) \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_byte_num, HNS3_BW_EVT_BYTE_NUM(_macro)), \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_time, HNS3_BW_EVT_TIME(_macro))
>> +#define HNS3_PMU_PPS_FLT_MODE_PAIR(_name, _macro) \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_packet_num, HNS3_PPS_EVT_PACKET_NUM(_macro)), \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_time, HNS3_PPS_EVT_TIME(_macro))
>> +#define HNS3_PMU_DLY_FLT_MODE_PAIR(_name, _macro) \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_time, HNS3_DLY_EVT_TIME(_macro)), \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_packet_num, HNS3_DLY_EVT_PACKET_NUM(_macro))
>> +#define HNS3_PMU_INTR_FLT_MODE_PAIR(_name, _macro) \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_intr_num, HNS3_INTR_EVT_INTR_NUM(_macro)), \
>> + HNS3_PMU_FLT_MODE_ATTR(_name##_time, HNS3_INTR_EVT_TIME(_macro))
>> +
>> +static u8 hns3_pmu_hw_filter_modes[] = {
>> + HNS3_PMU_HW_FILTER_GLOBAL,
>> + HNS3_PMU_HW_FILTER_PORT,
>> + HNS3_PMU_HW_FILTER_PORT_TC,
>> + HNS3_PMU_HW_FILTER_FUNC,
>> + HNS3_PMU_HW_FILTER_FUNC_QUEUE,
>> + HNS3_PMU_HW_FILTER_FUNC_INTR,
>> +};
>> +
>> +#define HNS3_PMU_SET_HW_FILTER(_hwc, _mode) \
>> + ((_hwc)->addr_filters = (void *)&hns3_pmu_hw_filter_modes[(_mode)])
>> +
>> +static ssize_t identifier_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
>> +
>> + return sysfs_emit(buf, "0x%x\n", hns3_pmu->identifier);
>> +}
>> +static DEVICE_ATTR_RO(identifier);
>> +
>> +static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
>> +
>> + return sysfs_emit(buf, "%d\n", hns3_pmu->on_cpu);
>> +}
>> +static DEVICE_ATTR_RO(cpumask);
>> +
>> +static ssize_t bdf_min_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
>> +
>> + return sysfs_emit(buf, "0x%4x\n", hns3_pmu->bdf_min);
>
> Is it worth formatting the bdf in the usual way ("b:d.f") rather than a
> plain 16-bit number.
>
Ok, I will modify it.
>> +}
>> +static DEVICE_ATTR_RO(bdf_min);
>> +
>> +static ssize_t bdf_max_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev));
>> +
>> + return sysfs_emit(buf, "0x%4x\n", hns3_pmu->bdf_max);
>
> Likewise.
>
Ok.
>> +static struct attribute *hns3_pmu_format_attr[] = {
>> + HNS3_PMU_FORMAT_ATTR(event, "config:0-16"),
>
> Earlier you split this into event, subevent, event_type and
> ext_counter_used. Why isn't that same structure used here? Ideally, you'd
> derive the strings and the accessors from the same set of definitions so
> you don't run the risk of them falling out of sync. I did that in
> arm_spe_pmu.c but it's not the prettiest code in the world.
>
Ok. I will modify it.
>> + HNS3_PMU_FORMAT_ATTR(port, "config1:0-3"),
>> + HNS3_PMU_FORMAT_ATTR(tc, "config1:4-7"),
>> + HNS3_PMU_FORMAT_ATTR(bdf, "config1:8-23"),
>> + HNS3_PMU_FORMAT_ATTR(queue, "config1:24-39"),
>> + HNS3_PMU_FORMAT_ATTR(intr, "config1:40-51"),
>> + HNS3_PMU_FORMAT_ATTR(global, "config1:52-52"),
>
> "52-52" is a bit weird; PMU drivers would usually just do "config1:52"
> for a single-bit field. Is perf tool happy with this?
>
Ok, I will modify it.
> Will
> .
>
Thanks very much!
Guangbin
.
Powered by blists - more mailing lists