Message ID | 1679885172-95021-2-git-send-email-renyu.zj@linux.alibaba.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1239758vqo; Sun, 26 Mar 2023 20:03:32 -0700 (PDT) X-Google-Smtp-Source: AKy350Y3ntdqthlIe5GWXspk7nFIDjVG9qJpdSpMHTgjKn06CTlKcy3MnUOOqZVyJqgo3BwiEYRr X-Received: by 2002:a17:902:cec6:b0:1a1:80ea:4364 with SMTP id d6-20020a170902cec600b001a180ea4364mr13404658plg.31.1679886211914; Sun, 26 Mar 2023 20:03:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679886211; cv=none; d=google.com; s=arc-20160816; b=A2kda8zi5wJkgCB3nl9NYFSfHFglSr2IxTFzEj1NqV20Z5AFBZp2DK38hy/Ew3qKxm VDYmBxi3ueS+T4SIJQUNcqunZpPrECb0V2DNmFs21t9o90kvT9gneobZIwLHG1ifRZDH xY1p0Pquial8O2syDZb+QJugJI3p36kwc31XrXMIKZOskoEzQZ1MqM9de/Jb3v59P538 F5S8jaqqmgkdf6AWtz3Wiwyz5ueDH7fdpSFiDLEXUJH51vuZw7bhnb5aGUFzKk0ORHDb yoVtaG3eB8keBbyZUvDmOBonvKwjv139oJEE9848eUWWoRkZxcCDgQCN8/JQvX5teBlN Es5w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:references:in-reply-to:message-id:date:subject :cc:to:from; bh=SMj6XemwbojuTBkDCQJuo0PwUTs7fzBbGI8VH7NLtgk=; b=G4lTFQo4nrkxPe4hwNMA0q2ExlXEkS81aUxAxo63Qk/eLl0qTnNl8pvf3ArbgJ0sXb wBmMKzpgzbC5CiS4Nxdzhl4Cbr+zP5aCIa6APwfe5uKIkG8IHwaw7nYN4xioTSNXp7NH D2w8n/PWecUj6T2r/AMIsr5EpybPySRoRu0wLfZWhUAoOeZ01yWk9o2v+JyCRpeMDejR jxGfh0uJONHsbRvuR/p+fiWP5L29NfNqPDdNsCcToKxhy34ONjalr9mb9ixsvhd2nXMO 44GgBA2eeog5T76u0lSUu9T39yN/6ZaRcsjJQejEmt0SDQw5vLM/ozkD8kMWJW47r5nf qZAA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e23-20020a63db17000000b0050beb3fe626si25729769pgg.13.2023.03.26.20.03.19; Sun, 26 Mar 2023 20:03:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229651AbjC0Cqj (ORCPT <rfc822;makky5685@gmail.com> + 99 others); Sun, 26 Mar 2023 22:46:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57472 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231928AbjC0Cq3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 26 Mar 2023 22:46:29 -0400 Received: from out30-100.freemail.mail.aliyun.com (out30-100.freemail.mail.aliyun.com [115.124.30.100]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 42A8444BD; Sun, 26 Mar 2023 19:46:27 -0700 (PDT) X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R191e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=ay29a033018046049;MF=renyu.zj@linux.alibaba.com;NM=1;PH=DS;RN=17;SR=0;TI=SMTPD_---0VeeqFPR_1679885183; Received: from srmbuffer011165236051.sqa.net(mailfrom:renyu.zj@linux.alibaba.com fp:SMTPD_---0VeeqFPR_1679885183) by smtp.aliyun-inc.com; Mon, 27 Mar 2023 10:46:24 +0800 From: Jing Zhang <renyu.zj@linux.alibaba.com> To: John Garry <john.g.garry@oracle.com>, Ian Rogers <irogers@google.com>, Will Deacon <will@kernel.org>, James Clark <james.clark@arm.com>, Mike Leach <mike.leach@linaro.org>, Leo Yan <leo.yan@linaro.org>, Mark Rutland <mark.rutland@arm.com>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>, Adrian Hunter <adrian.hunter@intel.com> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-perf-users@vger.kernel.org, Shuai Xue <xueshuai@linux.alibaba.com>, Zhuo Song <zhuo.song@linux.alibaba.com>, Jing Zhang <renyu.zj@linux.alibaba.com> Subject: [PATCH RFC 1/4] driver/perf: Add identifier sysfs file for CMN Date: Mon, 27 Mar 2023 10:46:09 +0800 Message-Id: <1679885172-95021-2-git-send-email-renyu.zj@linux.alibaba.com> X-Mailer: git-send-email 1.8.3.1 In-Reply-To: <1679885172-95021-1-git-send-email-renyu.zj@linux.alibaba.com> References: <1679885172-95021-1-git-send-email-renyu.zj@linux.alibaba.com> X-Spam-Status: No, score=-8.0 required=5.0 tests=ENV_AND_HDR_SPF_MATCH, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_PASS, UNPARSEABLE_RELAY,USER_IN_DEF_SPF_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761488364642102254?= X-GMAIL-MSGID: =?utf-8?q?1761488364642102254?= |
Series |
Add JSON metrics for arm CMN and Yitian710 DDR
|
|
Commit Message
Jing Zhang
March 27, 2023, 2:46 a.m. UTC
To allow userspace to identify the specific implementation of the device,
add an "identifier" sysfs file.
The perf tool can match the arm CMN metric through the identifier.
Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com>
---
drivers/perf/arm-cmn.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
Comments
On 27/03/2023 03:46, Jing Zhang wrote: > To allow userspace to identify the specific implementation of the device, > add an "identifier" sysfs file. > > The perf tool can match the arm CMN metric through the identifier. > > Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> > --- > drivers/perf/arm-cmn.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c > index c968986..0c138ad 100644 > --- a/drivers/perf/arm-cmn.c > +++ b/drivers/perf/arm-cmn.c > @@ -1168,10 +1168,53 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, > .attrs = arm_cmn_cpumask_attrs, > }; > > +static ssize_t arm_cmn_identifier_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); > + if (cmn->model == CMN700) { > + return sysfs_emit(buf, "%s\n", "CMN700"); Is it possible to have a pointer to this string in struct arm_cmn, such that we don't have to do this model to identifier lookup here? If-else chains like this are not scalable. BTW, does this HW have some HW identifier register, like iidr? I think that using that may be preferable. > + } > + else if (cmn->model == CMN650) { > + return sysfs_emit(buf, "%s\n", "CMN650"); I'd use lowercase names > + } > + else if (cmn->model == CMN600) { > + return sysfs_emit(buf, "%s\n", "CMN600"); > + } > + else if (cmn->model == CI700) { > + return sysfs_emit(buf, "%s\n", "CI700"); > + } > + return sysfs_emit(buf, "%s\n", "UNKNOWN"); can we have a "is_visble" attr to just no show this when unknown? > +} > + > +static umode_t arm_cmn_identifier_attr_visible(struct kobject *kobj, > + struct attribute *attr, int n) > +{ > + struct device *dev = kobj_to_dev(kobj); > + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); > + if (cmn->model <= 0) > + return 0; > + return attr->mode; > +}; > + > +static struct device_attribute arm_cmn_identifier_attr = > +__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); > + > +static struct attribute *arm_cmn_identifier_attrs[] = { > + &arm_cmn_identifier_attr.attr, > + NULL, nit: no need for trailing ',' on a sentinel > +}; > + > +static struct attribute_group arm_cmn_identifier_attr_group = { > + .attrs = arm_cmn_identifier_attrs, > + .is_visible = arm_cmn_identifier_attr_visible, > +}; > + > static const struct attribute_group *arm_cmn_attr_groups[] = { > &arm_cmn_event_attrs_group, > &arm_cmn_format_attrs_group, > &arm_cmn_cpumask_attr_group, > + &arm_cmn_identifier_attr_group, > NULL > }; >
在 2023/3/27 下午3:55, John Garry 写道: > On 27/03/2023 03:46, Jing Zhang wrote: >> To allow userspace to identify the specific implementation of the device, >> add an "identifier" sysfs file. >> >> The perf tool can match the arm CMN metric through the identifier. >> >> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> >> --- >> drivers/perf/arm-cmn.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> >> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c >> index c968986..0c138ad 100644 >> --- a/drivers/perf/arm-cmn.c >> +++ b/drivers/perf/arm-cmn.c >> @@ -1168,10 +1168,53 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, >> .attrs = arm_cmn_cpumask_attrs, >> }; >> +static ssize_t arm_cmn_identifier_show(struct device *dev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); >> + if (cmn->model == CMN700) { >> + return sysfs_emit(buf, "%s\n", "CMN700"); > > Is it possible to have a pointer to this string in struct arm_cmn, such that we don't have to do this model to identifier lookup here? If-else chains like this are not scalable. > Will do. > BTW, does this HW have some HW identifier register, like iidr? I think that using that may be preferable. > I didn't find the relevant identifier register. Do Illka and Robin know that there is such a register that can identify different CMN versions? Looking forward to your suggestions. >> + } >> + else if (cmn->model == CMN650) { >> + return sysfs_emit(buf, "%s\n", "CMN650"); > > I'd use lowercase names > Ok. >> + } >> + else if (cmn->model == CMN600) { >> + return sysfs_emit(buf, "%s\n", "CMN600"); >> + } >> + else if (cmn->model == CI700) { >> + return sysfs_emit(buf, "%s\n", "CI700"); >> + } >> + return sysfs_emit(buf, "%s\n", "UNKNOWN"); > > can we have a "is_visble" attr to just no show this when unknown? > Ok. >> +} >> + >> +static umode_t arm_cmn_identifier_attr_visible(struct kobject *kobj, >> + struct attribute *attr, int n) >> +{ >> + struct device *dev = kobj_to_dev(kobj); >> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); >> + if (cmn->model <= 0) >> + return 0; >> + return attr->mode; >> +}; >> + >> +static struct device_attribute arm_cmn_identifier_attr = >> +__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); >> + >> +static struct attribute *arm_cmn_identifier_attrs[] = { >> + &arm_cmn_identifier_attr.attr, >> + NULL, > > nit: no need for trailing ',' on a sentinel > Ok, Will do. >> +}; >> + >> +static struct attribute_group arm_cmn_identifier_attr_group = { >> + .attrs = arm_cmn_identifier_attrs, >> + .is_visible = arm_cmn_identifier_attr_visible, >> +}; >> + >> static const struct attribute_group *arm_cmn_attr_groups[] = { >> &arm_cmn_event_attrs_group, >> &arm_cmn_format_attrs_group, >> &arm_cmn_cpumask_attr_group, >> + &arm_cmn_identifier_attr_group, >> NULL >> }; >>
On 2023-03-29 12:53, Jing Zhang wrote: > > > 在 2023/3/27 下午3:55, John Garry 写道: >> On 27/03/2023 03:46, Jing Zhang wrote: >>> To allow userspace to identify the specific implementation of the device, >>> add an "identifier" sysfs file. >>> >>> The perf tool can match the arm CMN metric through the identifier. >>> >>> Signed-off-by: Jing Zhang <renyu.zj@linux.alibaba.com> >>> --- >>> drivers/perf/arm-cmn.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 43 insertions(+) >>> >>> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c >>> index c968986..0c138ad 100644 >>> --- a/drivers/perf/arm-cmn.c >>> +++ b/drivers/perf/arm-cmn.c >>> @@ -1168,10 +1168,53 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, >>> .attrs = arm_cmn_cpumask_attrs, >>> }; >>> +static ssize_t arm_cmn_identifier_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); >>> + if (cmn->model == CMN700) { >>> + return sysfs_emit(buf, "%s\n", "CMN700"); >> >> Is it possible to have a pointer to this string in struct arm_cmn, such that we don't have to do this model to identifier lookup here? If-else chains like this are not scalable. >> > Will do. > >> BTW, does this HW have some HW identifier register, like iidr? I think that using that may be preferable. >> > > I didn't find the relevant identifier register. > > Do Illka and Robin know that there is such a register that can identify different CMN versions? Looking forward to your suggestions. In principle the "part number" fields from CFGM_PERIPH_ID_0/1 are supposed to identify the model, but for various reasons I'm suspicious of that being unreliable (not least that no actual values are documented, only "configuration-dependent"). That's why I went down the route of making sure we have explicit ACPI/DT identifiers for every model. However, the model alone seems either too specific or not specific enough for a jevents identifier. The defined metrics are pretty trivial and should have no real reason not to be common to *any* CMN PMU where the underlying events are present. On the other hand, if we want to get down to the level of specific events in JSON then we'd need to consider the revision as well, since there are several events which only exist on certain revisions of a given model (but often are also common to later models). This actually foreshadows a question I was planning to bring up in the context of another driver I'm working on - for this one I would rather like to try using jevents rather than have to maintain another sprawl of event tables in a driver, but it's still going to have the same thing of wanting model/revision matching along the lines of what arm_cmn_event_attr_is_visible() is doing for CMN events. AFAICS this would need jevents to grow a rather more flexible way of encoding and matching identifiers, since having dozens of almost-identical copies of event definitions for every exact identifier value is clearly unworkable. Does anyone happen to have any thoughts or preferences around how that might be approached? >>> + } >>> + else if (cmn->model == CMN650) { >>> + return sysfs_emit(buf, "%s\n", "CMN650"); >> >> I'd use lowercase names >> > Ok. > >>> + } >>> + else if (cmn->model == CMN600) { >>> + return sysfs_emit(buf, "%s\n", "CMN600"); >>> + } >>> + else if (cmn->model == CI700) { >>> + return sysfs_emit(buf, "%s\n", "CI700"); >>> + } >>> + return sysfs_emit(buf, "%s\n", "UNKNOWN"); >> >> can we have a "is_visble" attr to just no show this when unknown? No need - it will never be unknown unless someone goes out of their way to break the probing code and/or match_data. >> > > Ok. > >>> +} >>> + >>> +static umode_t arm_cmn_identifier_attr_visible(struct kobject *kobj, >>> + struct attribute *attr, int n) >>> +{ >>> + struct device *dev = kobj_to_dev(kobj); >>> + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); >>> + if (cmn->model <= 0) >>> + return 0; >>> + return attr->mode; >>> +}; As above, "cmn->model <= 0" can never be true. Thanks, Robin. >>> + >>> +static struct device_attribute arm_cmn_identifier_attr = >>> +__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); >>> + >>> +static struct attribute *arm_cmn_identifier_attrs[] = { >>> + &arm_cmn_identifier_attr.attr, >>> + NULL, >> >> nit: no need for trailing ',' on a sentinel >> > > Ok, Will do. > >>> +}; >>> + >>> +static struct attribute_group arm_cmn_identifier_attr_group = { >>> + .attrs = arm_cmn_identifier_attrs, >>> + .is_visible = arm_cmn_identifier_attr_visible, >>> +}; >>> + >>> static const struct attribute_group *arm_cmn_attr_groups[] = { >>> &arm_cmn_event_attrs_group, >>> &arm_cmn_format_attrs_group, >>> &arm_cmn_cpumask_attr_group, >>> + &arm_cmn_identifier_attr_group, >>> NULL >>> }; >>>
On 29/03/2023 18:47, Robin Murphy wrote: >> Do Illka and Robin know that there is such a register that can >> identify different CMN versions? Looking forward to your suggestions. > > In principle the "part number" fields from CFGM_PERIPH_ID_0/1 are > supposed to identify the model, but for various reasons I'm suspicious > of that being unreliable (not least that no actual values are > documented, only "configuration-dependent"). That's why I went down the > route of making sure we have explicit ACPI/DT identifiers for every model. > > However, the model alone seems either too specific or not specific > enough for a jevents identifier. The defined metrics are pretty trivial > and should have no real reason not to be common to *any* CMN PMU where > the underlying events are present. On the other hand, if we want to get > down to the level of specific events in JSON then we'd need to consider > the revision as well, since there are several events which only exist on > certain revisions of a given model (but often are also common to later > models). > > This actually foreshadows a question I was planning to bring up in the > context of another driver I'm working on - for this one I would rather > like to try using jevents rather than have to maintain another sprawl of > event tables in a driver, but it's still going to have the same thing of > wanting model/revision matching along the lines of what > arm_cmn_event_attr_is_visible() is doing for CMN events. AFAICS this > would need jevents to grow a rather more flexible way of encoding and > matching identifiers, since having dozens of almost-identical copies of > event definitions for every exact identifier value is clearly > unworkable. This sort of problem has not occurred yet as perf tool only supports "system" events for a handful of SoCs so far :) > Does anyone happen to have any thoughts or preferences > around how that might be approached? > Currently the perf tool will just match system events based on the exact HW identifier and PMU name. However, if you consider PMCG PMU support as an example of possible area of improvement, it has a number of fixed events and a number of IMPDEF events. There should be no reason to need to describe in a separate JSON those fixed events for every instance of that PMU. So a simple change would be to teach perf tool that for certain fixed events we only need to match based on the PMU name. For others we need to match based on some identifier. If matching based on an identifier still leads to unwieldy amounts of tables, then maybe HW identifier wildcard matching may suit, like what is done for CPU events. Thanks, John
diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c index c968986..0c138ad 100644 --- a/drivers/perf/arm-cmn.c +++ b/drivers/perf/arm-cmn.c @@ -1168,10 +1168,53 @@ static ssize_t arm_cmn_cpumask_show(struct device *dev, .attrs = arm_cmn_cpumask_attrs, }; +static ssize_t arm_cmn_identifier_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); + if (cmn->model == CMN700) { + return sysfs_emit(buf, "%s\n", "CMN700"); + } + else if (cmn->model == CMN650) { + return sysfs_emit(buf, "%s\n", "CMN650"); + } + else if (cmn->model == CMN600) { + return sysfs_emit(buf, "%s\n", "CMN600"); + } + else if (cmn->model == CI700) { + return sysfs_emit(buf, "%s\n", "CI700"); + } + return sysfs_emit(buf, "%s\n", "UNKNOWN"); +} + +static umode_t arm_cmn_identifier_attr_visible(struct kobject *kobj, + struct attribute *attr, int n) +{ + struct device *dev = kobj_to_dev(kobj); + struct arm_cmn *cmn = to_cmn(dev_get_drvdata(dev)); + if (cmn->model <= 0) + return 0; + return attr->mode; +}; + +static struct device_attribute arm_cmn_identifier_attr = +__ATTR(identifier, 0444, arm_cmn_identifier_show, NULL); + +static struct attribute *arm_cmn_identifier_attrs[] = { + &arm_cmn_identifier_attr.attr, + NULL, +}; + +static struct attribute_group arm_cmn_identifier_attr_group = { + .attrs = arm_cmn_identifier_attrs, + .is_visible = arm_cmn_identifier_attr_visible, +}; + static const struct attribute_group *arm_cmn_attr_groups[] = { &arm_cmn_event_attrs_group, &arm_cmn_format_attrs_group, &arm_cmn_cpumask_attr_group, + &arm_cmn_identifier_attr_group, NULL };