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;
Date:   Fri, 2 Dec 2022 11:27:07 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Steven Rostedt <rostedt@goodmis.org>,
        Dan Williams <dan.j.williams@intel.com>
CC:     <ira.weiny@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dave Jiang <dave.jiang@intel.com>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>
Subject: Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command
Message-ID: <638a518b9c9b5_3cbe0294b5@dwillia2-xfh.jf.intel.com.notmuch>
References: <20221201002719.2596558-1-ira.weiny@intel.com>
 <20221201002719.2596558-4-ira.weiny@intel.com>
 <6389630036769_3cbe02947d@dwillia2-xfh.jf.intel.com.notmuch>
 <20221202083448.4b3b3254@gandalf.local.home>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221202083448.4b3b3254@gandalf.local.home>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?92cj4DVhwj6FSX+u5P89SGd+LjchKKotFzSlkRQ/TuXLmUJSK2uzdH8/aqLo?=
 =?us-ascii?Q?IFf+ZtmuP2Bn6japLSt2ux88bHDQaj3uhyeICc1AfrU/l/sm3kmK5GM4a0tG?=
 =?us-ascii?Q?twPQzR0WCJ1xSBrkIFGO/nLvXI1v8RlWCMdoXM9bHnKb3meqW88etpuPC8bf?=
 =?us-ascii?Q?nxN9V28xy2lbEMq0xtuxB987jcEldkOFPlrA0Uh/Y9GC8BCwH0M/KiL5Hhir?=
 =?us-ascii?Q?tygWGfH8WRNTBJWIoYiH02sYqkzizMBf1DVhgFYno6D5P80LKkcnJazGXZL7?=
 =?us-ascii?Q?1vCdvK4KNm35/pI6bkk6pJLQgJ+OSvmkxhdyf4lqdvutzNsRV6g/5KOt0Pc/?=
 =?us-ascii?Q?MmoWgMMXSRw9CBKX9BGxNXG/glWgjQ+jFHkOTg2FcabNz8CwCGobzUicz5pl?=
 =?us-ascii?Q?zz4RL0aq0/L3tZbpM9z+OV+iA5I8F9ySg9sieMUtLQTztQQw7KjfX3NpR7v7?=
 =?us-ascii?Q?ByEJtN5aJhaMIkyrbScHOnxVyVFjFFVYgDhtrXD+hQc80LUToBIF7r221SW9?=
 =?us-ascii?Q?vMQ+I096Lbn0CMZ+codpcSQCSPSkIxyIdwANXEX2tPi/JHTIZ/uL87gnzRIB?=
 =?us-ascii?Q?jBlsoS5syB8Pcgg394SAFspXodOQyrYW4JgY99r+AJRk7LsFt7oU+UopApwF?=
 =?us-ascii?Q?FBhE5G8BtCtzcM5iZ6VMxGXXkwRP875X0qeZ8sKMlJYpncDInn+bkO4riIoM?=
 =?us-ascii?Q?gyRlvkJt316aHn84SGsUnuCYzLqxOqye/UOrIhKIiRMQiRpArVhIw6xGbgBe?=
 =?us-ascii?Q?1EWcU4Rh94NRvNGmVO8CWr6jWqO/TWPEiq9QGLFVYcOwhKPrzRJCOH1kpwZw?=
 =?us-ascii?Q?W+znobahc1kvqBlutm57d3/mCVgt67VK4n3cwz9Fwoa5V+zpv03P42CGQLry?=
 =?us-ascii?Q?3XHfCa9ipURY1XpCycTWdOFC4GPhm/JSWvKFIGLD6mHmXfiSz/pCZ1MMEvya?=
 =?us-ascii?Q?tYn2skDlPIEek4C5LshEhgsAGhBwPBbby18B9ST7cu77zaYjxWCJg5TV8ylA?=
 =?us-ascii?Q?OMPruCDvoWcknsfPIiiwe/+HLEmLkDFAgGFrrg1kSLj0bbPvCgOD2biNDzDe?=
 =?us-ascii?Q?MYqUfw552Mx2XtVKJO3/102WINWAConF1PxnU1XrRIA6/iSiFOfU+gwoEuow?=
 =?us-ascii?Q?93x+/DngDR5BUPYhrTe4veg2vyVQyNC+I+kCT3Dnhm21gzC0AstFrtlUgvfM?=
 =?us-ascii?Q?5BAGtrLe9iI6p4DweUipVtFUpt+EnJXLWOpyYcMS2BLbrvJDtu/n575BE4oG?=
 =?us-ascii?Q?RCZa6ngBS17Lz9jcbNXwtq0pUnBZPenNs1P5WIKaTLyeEzN31/4RwDuFcrZ9?=
 =?us-ascii?Q?cdEmk2t+AtGbyHai9EugGMsU7u4ftBSIcz/mbg7yHpDUGhr+PK7ieD/z8N79?=
 =?us-ascii?Q?U4OUlHuugHYXm+HEyElUcD7FFSHYCaehVjRZGcw7NZVkGvXJrTJRhyu35sAL?=
 =?us-ascii?Q?tIli49kq/sc0fztZGONKuaOHK/yn3zrntHCMNBDiPPVwynH6Q7tZktHMFuqU?=
 =?us-ascii?Q?VnWKZ7/t9LwIpYUgp46Ck/qeR14sEnlNDgtBl3Bf2UJBYmN2UCOhw07fGzwv?=
 =?us-ascii?Q?SXHhlR679WXAtRDPqn5mRbdK215p9wGvjjcI4i4SLktMBLbPJYLc5nkIoX+U?=
 =?us-ascii?Q?FA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a0aebc22-d768-48a2-2f96-08dad49b352d
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1101MB2126.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Dec 2022 19:27:10.7425
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: aKDEAMWa/UhR32D+QPn3HF4SBrzeRYGusZSJwEGKMF13lSNKt6mrjBLTzpM2dp1PkqeBWRjznaYIkHJ9ZwuLDVXy90QcTjNuXVMhD4QbcBg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5142
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=ham 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

Steven Rostedt wrote:
> On Thu, 1 Dec 2022 18:29:20 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > >  static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> > >  				    enum cxl_event_log_type type)
> > >  {
> > > @@ -732,13 +769,22 @@ static void cxl_mem_get_records_log(struct cxl_dev_state *cxlds,
> > >  		}
> > >  
> > >  		nr_rec = le16_to_cpu(payload->record_count);
> > > -		if (trace_cxl_generic_event_enabled()) {
> > > +		if (nr_rec > 0) {
> > >  			int i;
> > >  
> > > -			for (i = 0; i < nr_rec; i++)
> > > -				trace_cxl_generic_event(dev_name(cxlds->dev),
> > > -							type,
> > > -							&payload->records[i]);
> > > +			if (trace_cxl_generic_event_enabled()) {  
> > 
> > Again, trace_cxl_generic_event_enabled() injects some awkward
> > formatting here to micro-optimize looping. Any performance benefit this
> > code might offer is likely offset by the extra human effort to read it.
> 
> This is commonly used throughout the kernel, and highly suggested for use to
> encapsulate any work being done only for tracing, when tracing is disabled.
> It uses static_braches/jump_labels which makes the loop into a 'nop' when
> tracing is off. That is, there is zero overhead for the for loop below (and
> there's not even a branch to skip it!)
> 
> But sure, if you really don't care as it's not a fast path, then keep it
> out. I like people to keep the habit of doing this, because otherwise it
> tends to creep into the fast paths.

Duly noted. It makes a lot of sense when you are tracing in a fast path
to skip any and all preamble code. In this case we are doing it after
doing a whole series of uncached PCI mmio reads with all the stalling
and serialization that implies. 

Speaking of which, this probably wants a cond_resched() after each loop
iteration.

I'll note it is also a tracepoint that is likely to be enabled most of
the time in production.