From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EC503C433EF for ; Wed, 15 Sep 2021 04:11:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C7EA560E74 for ; Wed, 15 Sep 2021 04:11:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230160AbhIOEMo (ORCPT ); Wed, 15 Sep 2021 00:12:44 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59816 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229450AbhIOEMn (ORCPT ); Wed, 15 Sep 2021 00:12:43 -0400 Received: from mail-pf1-x433.google.com (mail-pf1-x433.google.com [IPv6:2607:f8b0:4864:20::433]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BA8B9C061574 for ; Tue, 14 Sep 2021 21:11:25 -0700 (PDT) Received: by mail-pf1-x433.google.com with SMTP id j6so1515701pfa.4 for ; Tue, 14 Sep 2021 21:11:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Www1Sy/iGaECDjccUq89+Q4UC3DZFrVqkJMRs3jnAoA=; b=f5JtWLoOAwop2HOyICQphlrTyYGhbXJ4qzohTu9cza9ymnaDVDmPoOXlWMNlsYZ63j 48HThtIL3ulmBpXV0ywVISKr9usamlhlXBbyLfygAnd2803JLkoKDTpAeO5i/txnwsXF h1WNbhrz0rP6nIh4Wt1SElozQDPJW3xMcoZV8A50E0aFLtPG7ytbSJuvZGrXsRvmMfb5 fhOef76SGMSCcSV5p+Yo2XFgSLD3X5/lciKLKcK4hDQns1tgB33yK+6T5vk6ny5JXT4d wK4l0b1z1iXjLq1DziyR7UVbUgxWsUqeFEj2UwAB4EVUuk1iGb+WIa+zTL88czpx4uWl flgw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Www1Sy/iGaECDjccUq89+Q4UC3DZFrVqkJMRs3jnAoA=; b=7ov0zW2GfW2gWDRxrkIAdUHx9bLFkFNq00d214UtiLXHWR7NvDERRUD8wS/Ay71LBA N91dllB7XKwZsnpyCT1s+w20hU+xt9nIhqpuC9V6lkBy6PXe7oiSAlpRCqA0+nHJUTVp Q99UZJFpPY5djUkznJN+XNfkSLRqYMRdzPSJk319e+DBaQ4gXiX8fBaWgtJmeITRj7RW dDD8IxgIl+IesWrphkzj9IYxyA6vDyvxS1j8rcyVkGE1jhkEVLUmuF7ZPrxq7V96hDlN mID+9TwT3qBLI20+iruo001KNNSXTZNshMbU29nn6kPef5257zoqinbNtlnFjXBobvNL zzqw== X-Gm-Message-State: AOAM533MYV/5IW1ko0ukaeH8RVfG46BDOM+01GuxcAJ9ztw53lJhMBqV TQ1sACKahlmNbYPqXvdja5TMIZxxTmD3us2EV04cnQ== X-Google-Smtp-Source: ABdhPJzVXf0or9qBPzNfUFPKqhXw6K1+8mdqK8CY5lJZTuBV/KxnpGf4inQTjJwy/A58j34e85JTiH17oUDNfhLWDfc= X-Received: by 2002:a63:1e0e:: with SMTP id e14mr18563273pge.5.1631679085171; Tue, 14 Sep 2021 21:11:25 -0700 (PDT) MIME-Version: 1.0 References: <20210903050914.273525-1-kjain@linux.ibm.com> <20210903050914.273525-2-kjain@linux.ibm.com> In-Reply-To: From: Dan Williams Date: Tue, 14 Sep 2021 21:11:14 -0700 Message-ID: Subject: Re: [RESEND PATCH v4 1/4] drivers/nvdimm: Add nvdimm pmu structure To: kajoljain Cc: Michael Ellerman , linuxppc-dev , Linux NVDIMM , Linux Kernel Mailing List , Peter Zijlstra , "Weiny, Ira" , Vishal L Verma , maddy@linux.ibm.com, Santosh Sivaraj , "Aneesh Kumar K.V" , Vaibhav Jain , atrajeev@linux.vnet.ibm.com, Thomas Gleixner , rnsastry@linux.ibm.com Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 14, 2021 at 9:08 PM Dan Williams wrote: > > On Thu, Sep 9, 2021 at 12:56 AM kajoljain 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 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) > > >> Reviewed-by: Madhavan Srinivasan > > >> Tested-by: Nageswara R Sastry > > >> Signed-off-by: Kajol Jain > > >> --- > > >> 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 > > >> #include > > >> #include > > >> +#include > > >> +#include > > >> > > >> 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.