From: Dan Williams <dan.j.williams@intel.com>
To: kajoljain <kjain@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
Linux NVDIMM <nvdimm@lists.linux.dev>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
"Weiny, Ira" <ira.weiny@intel.com>,
Vishal L Verma <vishal.l.verma@intel.com>,
maddy@linux.ibm.com, Santosh Sivaraj <santosh@fossix.org>,
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
Vaibhav Jain <vaibhav@linux.ibm.com>,
atrajeev@linux.vnet.ibm.com, Thomas Gleixner <tglx@linutronix.de>,
rnsastry@linux.ibm.com
Subject: Re: [RESEND PATCH v4 1/4] drivers/nvdimm: Add nvdimm pmu structure
Date: Tue, 14 Sep 2021 21:11:14 -0700 [thread overview]
Message-ID: <CAPcyv4hZZX4WscR41PUSXZhDLPR6LuXHNRcJDO52gb+3MahYAA@mail.gmail.com> (raw)
In-Reply-To: <CAPcyv4hE4rh5R+8zy3X4gDJeuPzQ0oQHmHbe_pppgWB2_RjfAg@mail.gmail.com>
On Tue, Sep 14, 2021 at 9:08 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Thu, Sep 9, 2021 at 12:56 AM kajoljain <kjain@linux.ibm.com> wrote:
> >
> >
> >
> > On 9/8/21 3:29 AM, Dan Williams wrote:
> > > Hi Kajol,
> > >
> > > Apologies for the delay in responding to this series, some comments below:
> >
> > Hi Dan,
> > No issues, thanks for reviewing the patches.
> >
> > >
> > > On Thu, Sep 2, 2021 at 10:10 PM Kajol Jain <kjain@linux.ibm.com> wrote:
> > >>
> > >> A structure is added, called nvdimm_pmu, for performance
> > >> stats reporting support of nvdimm devices. It can be used to add
> > >> nvdimm pmu data such as supported events and pmu event functions
> > >> like event_init/add/read/del with cpu hotplug support.
> > >>
> > >> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > >> Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> > >> Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
> > >> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> > >> ---
> > >> include/linux/nd.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> > >> 1 file changed, 43 insertions(+)
> > >>
> > >> diff --git a/include/linux/nd.h b/include/linux/nd.h
> > >> index ee9ad76afbba..712499cf7335 100644
> > >> --- a/include/linux/nd.h
> > >> +++ b/include/linux/nd.h
> > >> @@ -8,6 +8,8 @@
> > >> #include <linux/ndctl.h>
> > >> #include <linux/device.h>
> > >> #include <linux/badblocks.h>
> > >> +#include <linux/platform_device.h>
> > >> +#include <linux/perf_event.h>
> > >>
> > >> enum nvdimm_event {
> > >> NVDIMM_REVALIDATE_POISON,
> > >> @@ -23,6 +25,47 @@ enum nvdimm_claim_class {
> > >> NVDIMM_CCLASS_UNKNOWN,
> > >> };
> > >>
> > >> +/* Event attribute array index */
> > >> +#define NVDIMM_PMU_FORMAT_ATTR 0
> > >> +#define NVDIMM_PMU_EVENT_ATTR 1
> > >> +#define NVDIMM_PMU_CPUMASK_ATTR 2
> > >> +#define NVDIMM_PMU_NULL_ATTR 3
> > >> +
> > >> +/**
> > >> + * struct nvdimm_pmu - data structure for nvdimm perf driver
> > >> + *
> > >> + * @name: name of the nvdimm pmu device.
> > >> + * @pmu: pmu data structure for nvdimm performance stats.
> > >> + * @dev: nvdimm device pointer.
> > >> + * @functions(event_init/add/del/read): platform specific pmu functions.
> > >
> > > This is not valid kernel-doc:
> > >
> > > include/linux/nd.h:67: warning: Function parameter or member
> > > 'event_init' not described in 'nvdimm_pmu'
> > > include/linux/nd.h:67: warning: Function parameter or member 'add' not
> > > described in 'nvdimm_pmu'
> > > include/linux/nd.h:67: warning: Function parameter or member 'del' not
> > > described in 'nvdimm_pmu'
> > > include/linux/nd.h:67: warning: Function parameter or member 'read'
> > > not described in 'nvdimm_pmu'
> > >
> > > ...but I think rather than fixing those up 'struct nvdimm_pmu' should be pruned.
> > >
> > > It's not clear to me that it is worth the effort to describe these
> > > details to the nvdimm core which is just going to turn around and call
> > > the pmu core. I'd just as soon have the driver call the pmu core
> > > directly, optionally passing in attributes and callbacks that come
> > > from the nvdimm core and/or the nvdimm provider.
> >
> > The intend for adding these callbacks(event_init/add/del/read) is to give
> > flexibility to the nvdimm core to add some common checks/routines if required
> > in the future. Those checks can be common for all architecture with still having the
> > ability to call arch/platform specific driver code to use its own routines.
> >
> > But as you said, currently we don't have any common checks and it directly
> > calling platform specific code, so we can get rid of it.
> > Should we remove this part for now?
>
> Yes, lets go direct to the perf api for now and await the need for a
> common core wrapper to present itself.
>
> >
> >
> > >
> > > Otherwise it's also not clear which of these structure members are
> > > used at runtime vs purely used as temporary storage to pass parameters
> > > to the pmu core.
> > >
> > >> + * @attr_groups: data structure for events, formats and cpumask
> > >> + * @cpu: designated cpu for counter access.
> > >> + * @node: node for cpu hotplug notifier link.
> > >> + * @cpuhp_state: state for cpu hotplug notification.
> > >> + * @arch_cpumask: cpumask to get designated cpu for counter access.
> > >> + */
> > >> +struct nvdimm_pmu {
> > >> + const char *name;
> > >> + struct pmu pmu;
> > >> + struct device *dev;
> > >> + int (*event_init)(struct perf_event *event);
> > >> + int (*add)(struct perf_event *event, int flags);
> > >> + void (*del)(struct perf_event *event, int flags);
> > >> + void (*read)(struct perf_event *event);
> > >> + /*
> > >> + * Attribute groups for the nvdimm pmu. Index 0 used for
> > >> + * format attribute, index 1 used for event attribute,
> > >> + * index 2 used for cpusmask attribute and index 3 kept as NULL.
> > >> + */
> > >> + const struct attribute_group *attr_groups[4];
> > >
> > > Following from above, I'd rather this was organized as static
> > > attributes with an is_visible() helper for the groups for any dynamic
> > > aspects. That mirrors the behavior of nvdimm_create() and allows for
> > > device drivers to compose the attribute groups from a core set and /
> > > or a provider specific set.
> >
> > Since we don't have any common events right now, Can I use papr
> > attributes directly or should we create dummy events for common thing and
> > then merged it with papr event list.
>
> Just use papr events directly.
That is to say...I think if another implementation followed it should
try to match as many common event names as papr_scm picked, and
possibly extend with its own rather than start with a papr_scm
specific namespace for everything.
WARNING: multiple messages have this Message-ID (diff)
From: Dan Williams <dan.j.williams@intel.com>
To: kajoljain <kjain@linux.ibm.com>
Cc: Linux NVDIMM <nvdimm@lists.linux.dev>,
Santosh Sivaraj <santosh@fossix.org>,
maddy@linux.ibm.com, "Weiny, Ira" <ira.weiny@intel.com>,
rnsastry@linux.ibm.com, Peter Zijlstra <peterz@infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
atrajeev@linux.vnet.ibm.com,
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
Vishal L Verma <vishal.l.verma@intel.com>,
Vaibhav Jain <vaibhav@linux.ibm.com>,
Thomas Gleixner <tglx@linutronix.de>,
linuxppc-dev <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [RESEND PATCH v4 1/4] drivers/nvdimm: Add nvdimm pmu structure
Date: Tue, 14 Sep 2021 21:11:14 -0700 [thread overview]
Message-ID: <CAPcyv4hZZX4WscR41PUSXZhDLPR6LuXHNRcJDO52gb+3MahYAA@mail.gmail.com> (raw)
In-Reply-To: <CAPcyv4hE4rh5R+8zy3X4gDJeuPzQ0oQHmHbe_pppgWB2_RjfAg@mail.gmail.com>
On Tue, Sep 14, 2021 at 9:08 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Thu, Sep 9, 2021 at 12:56 AM kajoljain <kjain@linux.ibm.com> wrote:
> >
> >
> >
> > On 9/8/21 3:29 AM, Dan Williams wrote:
> > > Hi Kajol,
> > >
> > > Apologies for the delay in responding to this series, some comments below:
> >
> > Hi Dan,
> > No issues, thanks for reviewing the patches.
> >
> > >
> > > On Thu, Sep 2, 2021 at 10:10 PM Kajol Jain <kjain@linux.ibm.com> wrote:
> > >>
> > >> A structure is added, called nvdimm_pmu, for performance
> > >> stats reporting support of nvdimm devices. It can be used to add
> > >> nvdimm pmu data such as supported events and pmu event functions
> > >> like event_init/add/read/del with cpu hotplug support.
> > >>
> > >> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > >> Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> > >> Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
> > >> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
> > >> ---
> > >> include/linux/nd.h | 43 +++++++++++++++++++++++++++++++++++++++++++
> > >> 1 file changed, 43 insertions(+)
> > >>
> > >> diff --git a/include/linux/nd.h b/include/linux/nd.h
> > >> index ee9ad76afbba..712499cf7335 100644
> > >> --- a/include/linux/nd.h
> > >> +++ b/include/linux/nd.h
> > >> @@ -8,6 +8,8 @@
> > >> #include <linux/ndctl.h>
> > >> #include <linux/device.h>
> > >> #include <linux/badblocks.h>
> > >> +#include <linux/platform_device.h>
> > >> +#include <linux/perf_event.h>
> > >>
> > >> enum nvdimm_event {
> > >> NVDIMM_REVALIDATE_POISON,
> > >> @@ -23,6 +25,47 @@ enum nvdimm_claim_class {
> > >> NVDIMM_CCLASS_UNKNOWN,
> > >> };
> > >>
> > >> +/* Event attribute array index */
> > >> +#define NVDIMM_PMU_FORMAT_ATTR 0
> > >> +#define NVDIMM_PMU_EVENT_ATTR 1
> > >> +#define NVDIMM_PMU_CPUMASK_ATTR 2
> > >> +#define NVDIMM_PMU_NULL_ATTR 3
> > >> +
> > >> +/**
> > >> + * struct nvdimm_pmu - data structure for nvdimm perf driver
> > >> + *
> > >> + * @name: name of the nvdimm pmu device.
> > >> + * @pmu: pmu data structure for nvdimm performance stats.
> > >> + * @dev: nvdimm device pointer.
> > >> + * @functions(event_init/add/del/read): platform specific pmu functions.
> > >
> > > This is not valid kernel-doc:
> > >
> > > include/linux/nd.h:67: warning: Function parameter or member
> > > 'event_init' not described in 'nvdimm_pmu'
> > > include/linux/nd.h:67: warning: Function parameter or member 'add' not
> > > described in 'nvdimm_pmu'
> > > include/linux/nd.h:67: warning: Function parameter or member 'del' not
> > > described in 'nvdimm_pmu'
> > > include/linux/nd.h:67: warning: Function parameter or member 'read'
> > > not described in 'nvdimm_pmu'
> > >
> > > ...but I think rather than fixing those up 'struct nvdimm_pmu' should be pruned.
> > >
> > > It's not clear to me that it is worth the effort to describe these
> > > details to the nvdimm core which is just going to turn around and call
> > > the pmu core. I'd just as soon have the driver call the pmu core
> > > directly, optionally passing in attributes and callbacks that come
> > > from the nvdimm core and/or the nvdimm provider.
> >
> > The intend for adding these callbacks(event_init/add/del/read) is to give
> > flexibility to the nvdimm core to add some common checks/routines if required
> > in the future. Those checks can be common for all architecture with still having the
> > ability to call arch/platform specific driver code to use its own routines.
> >
> > But as you said, currently we don't have any common checks and it directly
> > calling platform specific code, so we can get rid of it.
> > Should we remove this part for now?
>
> Yes, lets go direct to the perf api for now and await the need for a
> common core wrapper to present itself.
>
> >
> >
> > >
> > > Otherwise it's also not clear which of these structure members are
> > > used at runtime vs purely used as temporary storage to pass parameters
> > > to the pmu core.
> > >
> > >> + * @attr_groups: data structure for events, formats and cpumask
> > >> + * @cpu: designated cpu for counter access.
> > >> + * @node: node for cpu hotplug notifier link.
> > >> + * @cpuhp_state: state for cpu hotplug notification.
> > >> + * @arch_cpumask: cpumask to get designated cpu for counter access.
> > >> + */
> > >> +struct nvdimm_pmu {
> > >> + const char *name;
> > >> + struct pmu pmu;
> > >> + struct device *dev;
> > >> + int (*event_init)(struct perf_event *event);
> > >> + int (*add)(struct perf_event *event, int flags);
> > >> + void (*del)(struct perf_event *event, int flags);
> > >> + void (*read)(struct perf_event *event);
> > >> + /*
> > >> + * Attribute groups for the nvdimm pmu. Index 0 used for
> > >> + * format attribute, index 1 used for event attribute,
> > >> + * index 2 used for cpusmask attribute and index 3 kept as NULL.
> > >> + */
> > >> + const struct attribute_group *attr_groups[4];
> > >
> > > Following from above, I'd rather this was organized as static
> > > attributes with an is_visible() helper for the groups for any dynamic
> > > aspects. That mirrors the behavior of nvdimm_create() and allows for
> > > device drivers to compose the attribute groups from a core set and /
> > > or a provider specific set.
> >
> > Since we don't have any common events right now, Can I use papr
> > attributes directly or should we create dummy events for common thing and
> > then merged it with papr event list.
>
> Just use papr events directly.
That is to say...I think if another implementation followed it should
try to match as many common event names as papr_scm picked, and
possibly extend with its own rather than start with a papr_scm
specific namespace for everything.
next prev parent reply other threads:[~2021-09-15 4:11 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-03 5:09 [RESEND PATCH v4 0/4] Add perf interface to expose nvdimm Kajol Jain
2021-09-03 5:09 ` Kajol Jain
2021-09-03 5:09 ` [RESEND PATCH v4 1/4] drivers/nvdimm: Add nvdimm pmu structure Kajol Jain
2021-09-03 5:09 ` Kajol Jain
2021-09-07 21:59 ` Dan Williams
2021-09-07 21:59 ` Dan Williams
2021-09-09 7:55 ` kajoljain
2021-09-09 7:55 ` kajoljain
2021-09-15 4:08 ` Dan Williams
2021-09-15 4:08 ` Dan Williams
2021-09-15 4:11 ` Dan Williams [this message]
2021-09-15 4:11 ` Dan Williams
2021-09-03 5:09 ` [RESEND PATCH v4 2/4] drivers/nvdimm: Add perf interface to expose nvdimm performance stats Kajol Jain
2021-09-03 5:09 ` Kajol Jain
2021-09-03 12:32 ` kernel test robot
2021-09-03 12:32 ` kernel test robot
2021-09-03 12:32 ` kernel test robot
2021-09-03 15:19 ` kernel test robot
2021-09-03 15:19 ` kernel test robot
2021-09-03 15:19 ` kernel test robot
2021-09-04 6:38 ` kajoljain
2021-09-04 6:38 ` kajoljain
2021-09-04 6:38 ` kajoljain
2021-09-03 15:19 ` [RFC PATCH] drivers/nvdimm: nvdimm_pmu_free_hotplug_memory() can be static kernel test robot
2021-09-03 15:19 ` kernel test robot
2021-09-03 15:19 ` kernel test robot
2021-09-04 6:39 ` kajoljain
2021-09-04 6:39 ` kajoljain
2021-09-04 6:39 ` kajoljain
2021-09-03 5:09 ` [RESEND PATCH v4 3/4] powerpc/papr_scm: Add perf interface support Kajol Jain
2021-09-03 5:09 ` Kajol Jain
2021-09-03 5:09 ` [RESEND PATCH v4 4/4] powerpc/papr_scm: Document papr_scm sysfs event format entries Kajol Jain
2021-09-03 5:09 ` Kajol Jain
2021-09-08 1:03 ` Dan Williams
2021-09-08 1:03 ` Dan Williams
2021-09-09 8:03 ` kajoljain
2021-09-09 8:03 ` kajoljain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPcyv4hZZX4WscR41PUSXZhDLPR6LuXHNRcJDO52gb+3MahYAA@mail.gmail.com \
--to=dan.j.williams@intel.com \
--cc=aneesh.kumar@linux.ibm.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=ira.weiny@intel.com \
--cc=kjain@linux.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=maddy@linux.ibm.com \
--cc=mpe@ellerman.id.au \
--cc=nvdimm@lists.linux.dev \
--cc=peterz@infradead.org \
--cc=rnsastry@linux.ibm.com \
--cc=santosh@fossix.org \
--cc=tglx@linutronix.de \
--cc=vaibhav@linux.ibm.com \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.