| Message ID | 1621417919-6632-2-git-send-email-liuqi115@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers/perf: Use general macro to simplify event attributes | expand |
On Wed, May 19, 2021 at 05:51:51PM +0800, Qi Liu wrote: > Similar EVENT_ATTR macros are defined in many PMU drivers, > like HiSilicon PMU driver, Arm PMU driver, Arm SMMU PMU > driver. So Add a generic macro to simplify code. > > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Signed-off-by: Qi Liu <liuqi115@huawei.com> > --- > include/linux/perf_event.h | 6 ++++++ > kernel/events/core.c | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > index f5a6a2f..d0aa74e 100644 > --- a/include/linux/perf_event.h > +++ b/include/linux/perf_event.h > @@ -1576,6 +1576,12 @@ static struct perf_pmu_events_attr _var = { \ > .event_str = _str, \ > }; > > +#define PMU_EVENT_ATTR_ID(_name, _id) \ > + (&((struct perf_pmu_events_attr[]) { \ > + { .attr = __ATTR(_name, 0444, perf_event_sysfs_show, NULL), \ > + .id = _id, } \ > + })[0].attr.attr) > + > #define PMU_FORMAT_ATTR(_name, _format) \ > static ssize_t \ > _name##_show(struct device *dev, \ > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 0ac818b..330d9cc 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -13295,6 +13295,8 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr, > > if (pmu_attr->event_str) > return sprintf(page, "%s\n", pmu_attr->event_str); > + else > + return sprintf(page, "config=%#llx\n", pmu_attr->id); I think it's a really bad idea to hardcode this here. For example, I think this patch series breaks user ABI for the SMMU PMU which used to print: "event=0x%02llx\n" and by the looks of it many of the other conversions are unsound too. I'm all for a common macro, but the string needs to be determined by the driver. Will
Hi Will, Thanks for reviewing this patch. On 2021/6/1 21:10, Will Deacon wrote: > On Wed, May 19, 2021 at 05:51:51PM +0800, Qi Liu wrote: >> Similar EVENT_ATTR macros are defined in many PMU drivers, >> like HiSilicon PMU driver, Arm PMU driver, Arm SMMU PMU >> driver. So Add a generic macro to simplify code. >> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Signed-off-by: Qi Liu <liuqi115@huawei.com> >> --- >> include/linux/perf_event.h | 6 ++++++ >> kernel/events/core.c | 2 ++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >> index f5a6a2f..d0aa74e 100644 >> --- a/include/linux/perf_event.h >> +++ b/include/linux/perf_event.h >> @@ -1576,6 +1576,12 @@ static struct perf_pmu_events_attr _var = { \ >> .event_str = _str, \ >> }; >> >> +#define PMU_EVENT_ATTR_ID(_name, _id) \ >> + (&((struct perf_pmu_events_attr[]) { \ >> + { .attr = __ATTR(_name, 0444, perf_event_sysfs_show, NULL), \ >> + .id = _id, } \ >> + })[0].attr.attr) >> + >> #define PMU_FORMAT_ATTR(_name, _format) \ >> static ssize_t \ >> _name##_show(struct device *dev, \ >> diff --git a/kernel/events/core.c b/kernel/events/core.c >> index 0ac818b..330d9cc 100644 >> --- a/kernel/events/core.c >> +++ b/kernel/events/core.c >> @@ -13295,6 +13295,8 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr, >> >> if (pmu_attr->event_str) >> return sprintf(page, "%s\n", pmu_attr->event_str); >> + else >> + return sprintf(page, "config=%#llx\n", pmu_attr->id); > > I think it's a really bad idea to hardcode this here. For example, I think > this patch series breaks user ABI for the SMMU PMU which used to print: > > "event=0x%02llx\n" > > and by the looks of it many of the other conversions are unsound too. > Got it, so I'll use pmu_attr->event_str here, for example, SMMU_EVENT_ATTR(cycles, "event=0x00") As PMU_EVENT_ATTR_STRING is already defined in linux/perf_event.h，and is used in drivers of multi architectures, add a new common macro might be better than modify PMU_EVENT_ATTR_STRING. Do you have any suggestion about the name of new common macro? Thanks, Qi > I'm all for a common macro, but the string needs to be determined by the > driver. > > Will > . >
On Wed, Jun 02, 2021 at 04:45:23PM +0800, liuqi (BA) wrote: > > Hi Will, > > Thanks for reviewing this patch. > > On 2021/6/1 21:10, Will Deacon wrote: > > On Wed, May 19, 2021 at 05:51:51PM +0800, Qi Liu wrote: > > > Similar EVENT_ATTR macros are defined in many PMU drivers, > > > like HiSilicon PMU driver, Arm PMU driver, Arm SMMU PMU > > > driver. So Add a generic macro to simplify code. > > > > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > Cc: Ingo Molnar <mingo@redhat.com> > > > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > > > Signed-off-by: Qi Liu <liuqi115@huawei.com> > > > --- > > > include/linux/perf_event.h | 6 ++++++ > > > kernel/events/core.c | 2 ++ > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h > > > index f5a6a2f..d0aa74e 100644 > > > --- a/include/linux/perf_event.h > > > +++ b/include/linux/perf_event.h > > > @@ -1576,6 +1576,12 @@ static struct perf_pmu_events_attr _var = { \ > > > .event_str = _str, \ > > > }; > > > +#define PMU_EVENT_ATTR_ID(_name, _id) \ > > > + (&((struct perf_pmu_events_attr[]) { \ > > > + { .attr = __ATTR(_name, 0444, perf_event_sysfs_show, NULL), \ > > > + .id = _id, } \ > > > + })[0].attr.attr) > > > + > > > #define PMU_FORMAT_ATTR(_name, _format) \ > > > static ssize_t \ > > > _name##_show(struct device *dev, \ > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > > index 0ac818b..330d9cc 100644 > > > --- a/kernel/events/core.c > > > +++ b/kernel/events/core.c > > > @@ -13295,6 +13295,8 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr, > > > if (pmu_attr->event_str) > > > return sprintf(page, "%s\n", pmu_attr->event_str); > > > + else > > > + return sprintf(page, "config=%#llx\n", pmu_attr->id); > > > > I think it's a really bad idea to hardcode this here. For example, I think > > this patch series breaks user ABI for the SMMU PMU which used to print: > > > > "event=0x%02llx\n" > > > > and by the looks of it many of the other conversions are unsound too. > > > Got it, so I'll use pmu_attr->event_str here, for example, > SMMU_EVENT_ATTR(cycles, "event=0x00") You could, but honestly I don't really see this being any better than the current code. The advantage of using "event=0x%02llx\n" is that things are consistent by construction and therefore userspace can parse the information easily. We lose that if we just hardcode the string and it's error-prone to extend. Will
Hi Will, On 2021/6/2 17:49, Will Deacon wrote: > On Wed, Jun 02, 2021 at 04:45:23PM +0800, liuqi (BA) wrote: >> >> Hi Will, >> >> Thanks for reviewing this patch. >> >> On 2021/6/1 21:10, Will Deacon wrote: >>> On Wed, May 19, 2021 at 05:51:51PM +0800, Qi Liu wrote: >>>> Similar EVENT_ATTR macros are defined in many PMU drivers, >>>> like HiSilicon PMU driver, Arm PMU driver, Arm SMMU PMU >>>> driver. So Add a generic macro to simplify code. >>>> >>>> Cc: Peter Zijlstra <peterz@infradead.org> >>>> Cc: Ingo Molnar <mingo@redhat.com> >>>> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> >>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >>>> Signed-off-by: Qi Liu <liuqi115@huawei.com> >>>> --- >>>> include/linux/perf_event.h | 6 ++++++ >>>> kernel/events/core.c | 2 ++ >>>> 2 files changed, 8 insertions(+) >>>> >>>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h >>>> index f5a6a2f..d0aa74e 100644 >>>> --- a/include/linux/perf_event.h >>>> +++ b/include/linux/perf_event.h >>>> @@ -1576,6 +1576,12 @@ static struct perf_pmu_events_attr _var = { \ >>>> .event_str = _str, \ >>>> }; >>>> +#define PMU_EVENT_ATTR_ID(_name, _id) \ >>>> + (&((struct perf_pmu_events_attr[]) { \ >>>> + { .attr = __ATTR(_name, 0444, perf_event_sysfs_show, NULL), \ >>>> + .id = _id, } \ >>>> + })[0].attr.attr) >>>> + >>>> #define PMU_FORMAT_ATTR(_name, _format) \ >>>> static ssize_t \ >>>> _name##_show(struct device *dev, \ >>>> diff --git a/kernel/events/core.c b/kernel/events/core.c >>>> index 0ac818b..330d9cc 100644 >>>> --- a/kernel/events/core.c >>>> +++ b/kernel/events/core.c >>>> @@ -13295,6 +13295,8 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr, >>>> if (pmu_attr->event_str) >>>> return sprintf(page, "%s\n", pmu_attr->event_str); >>>> + else >>>> + return sprintf(page, "config=%#llx\n", pmu_attr->id); >>> >>> I think it's a really bad idea to hardcode this here. For example, I think >>> this patch series breaks user ABI for the SMMU PMU which used to print: >>> >>> "event=0x%02llx\n" >>> >>> and by the looks of it many of the other conversions are unsound too. >>> >> Got it, so I'll use pmu_attr->event_str here, for example, >> SMMU_EVENT_ATTR(cycles, "event=0x00") > > You could, but honestly I don't really see this being any better than the > current code. The advantage of using "event=0x%02llx\n" is that things are > consistent by construction and therefore userspace can parse the information > easily. We lose that if we just hardcode the string and it's error-prone to > extend. > > Will Got it, thanks. So how about adding a common macro like this: #define PMU_EVENT_ATTR_ID(_name,_func, _id) \ (&((struct perf_pmu_events_attr[]) { \ { .attr = __ATTR(_name, 0444, _func, NULL), \ .id = _id, } \ })[0].attr.attr) Drivers could define thir private macro, like SMMU_PMU_EVENT_ATTR, and use their private event_show() functions if they need, or use the common function perf_event_sysfs_show(). Thanks, Qi > . >
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h index f5a6a2f..d0aa74e 100644 --- a/include/linux/perf_event.h +++ b/include/linux/perf_event.h @@ -1576,6 +1576,12 @@ static struct perf_pmu_events_attr _var = { \ .event_str = _str, \ }; +#define PMU_EVENT_ATTR_ID(_name, _id) \ + (&((struct perf_pmu_events_attr[]) { \ + { .attr = __ATTR(_name, 0444, perf_event_sysfs_show, NULL), \ + .id = _id, } \ + })[0].attr.attr) + #define PMU_FORMAT_ATTR(_name, _format) \ static ssize_t \ _name##_show(struct device *dev, \ diff --git a/kernel/events/core.c b/kernel/events/core.c index 0ac818b..330d9cc 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -13295,6 +13295,8 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr, if (pmu_attr->event_str) return sprintf(page, "%s\n", pmu_attr->event_str); + else + return sprintf(page, "config=%#llx\n", pmu_attr->id); return 0; }
Similar EVENT_ATTR macros are defined in many PMU drivers, like HiSilicon PMU driver, Arm PMU driver, Arm SMMU PMU driver. So Add a generic macro to simplify code. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Signed-off-by: Qi Liu <liuqi115@huawei.com> --- include/linux/perf_event.h | 6 ++++++ kernel/events/core.c | 2 ++ 2 files changed, 8 insertions(+)