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 13:28:19 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>
CC:     Steven Rostedt <rostedt@goodmis.org>,
        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: <Y4pt87Lr2D5Hkycj@iweiny-desk3>
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>
 <638a518b9c9b5_3cbe0294b5@dwillia2-xfh.jf.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <638a518b9c9b5_3cbe0294b5@dwillia2-xfh.jf.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?azJOU7/uxdTAjWfXu9VNldhjiT9jkwv0Tm9ggGsNEHiBsRpJvTC2xHsnIdJU?=
 =?us-ascii?Q?4H7N+ZBQN+R1vkI1vjuGTCSKURLN3iykVTWkSnEl8GRYWVmQ2tjkYJg6StBT?=
 =?us-ascii?Q?+JNwvJKqPMPomSH6jt1WFrio+hATQot3ySvg3kV8HqnxJZ7I5mmacsUERpwB?=
 =?us-ascii?Q?R2gnqkwgDMavn/G+qgPjVuVKLB15Hf1QFa3ngz1czusiWgf5nzMXafswOcNy?=
 =?us-ascii?Q?euhrxZKX85WOZBqksHm9JoHBOAItSnFU+4VqyFhbDgwaeUtniz94+TSTLimS?=
 =?us-ascii?Q?2VR3vNdwCa+hpgRXo/OP7RGOp/Bj7evDx4kVjO3mwSHSDGpcSCOBgUwjW9Pd?=
 =?us-ascii?Q?slURisqsU4I2cEZOKjuaKDWXRCSSOCcUZFmYta/2BxPgj5Kyol7dfXKs9UjL?=
 =?us-ascii?Q?8C5A17DvCT7I67Rn0JQt2tyDdV0fRbPf2jPS2c+Fc5beknDpSwygSUMHFba6?=
 =?us-ascii?Q?RYchwynOEu/7e6pjdyeczgHLIElKgNZoHoGEVWGzUBdnb1Afpr5/9PXwPPgM?=
 =?us-ascii?Q?nZDT2xMYR5Dag8QPLC2gq1uOkbX/a8N5DGn40NYbfXhEni+Am2RrfZTgu3nL?=
 =?us-ascii?Q?SosYd8UPzWWxeSBjgpMF0oJgq61lekGzbnf6LgA8Kchcg66I82CLnNNW3NkS?=
 =?us-ascii?Q?U2/Zd5VH4q0SxC7xj5EpbfFbMMHlEjC0PFIbCoTbYvpnmOoWdMPeJcUAr73E?=
 =?us-ascii?Q?nXg3MzvNpHd79c8EglcBAh0mJT5xXLFsTT52Y+zItHf1CxZMttWwDtBuKTDr?=
 =?us-ascii?Q?yvbK7Dp6TM7oEeLKdoTbsB+8GyfbcsFv3Uact23FXUJmc/37Wi7l8QvM6bre?=
 =?us-ascii?Q?D27o5kn5/XLTe3wm5hjxo8SCRqenoPkwzUj+kBHYh1UK0PoRvXK3B4O4a2Lx?=
 =?us-ascii?Q?CbRd2QOjNI7DOa9JBVkM94b9p0OZJWlSvgnSK1z7Z3y9lhaY88QlvJpi2Nl6?=
 =?us-ascii?Q?dtm6tAqVhwVIPYkvUwxvFJFfihlymdt5BxJgHNZIZihd/x9KiCPBWhYs5NnE?=
 =?us-ascii?Q?dTx+G9SUBGWzbsfxcZsAf28hrEGxv51RXCAJJurka9LNalrwyjE/7dP0viPd?=
 =?us-ascii?Q?xzg+e7jD5U05QGmoIN4QGiD3OM1/CjKYZ3qSP+JNCpxXRyyM+5pK75ERaJm1?=
 =?us-ascii?Q?j9c582NA/d4iOvc6N4uFN07o5fOJbVsTPmhTi1fgUtg88TTFQMpn1wj3KqQz?=
 =?us-ascii?Q?rGtk9lZTZPRSKHTFSYPTNXdpL/MDOiHxmakP5LdgcNun59qTC1b/KRmRGUf0?=
 =?us-ascii?Q?pseUlU8ci1AwlouDKdLQ+zrZnGR8n5bZEaceRyA8AhRPFER1Uw398HKuO+bM?=
 =?us-ascii?Q?e/pvb7iszC551SfcI9S9QB9RFIZb4o261kkgffsLo9h3VYGfP58+QBNzgZC3?=
 =?us-ascii?Q?PoeHlTbNmTJ26ZUkxq9feK6jQv1lj2xc6wn4/6H4g8Ms2tVRBsrLkPKPiOxf?=
 =?us-ascii?Q?FvR1ryPN7iVtLGeC4R5e6Zl3OTsFDsU5M24CO7slZosrwgf3Gw2gACrFgnPW?=
 =?us-ascii?Q?xC9QCps54pAqmoZIUpp8iWgIsDsEP7Jl4UD2yaFD6+3nf48BH+NNcDIyAGYJ?=
 =?us-ascii?Q?WipHwzpZapuuVEILe1Fip+biz0xyhJ0QEjiTlYZ3?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bb684a37-6308-4f34-f6e5-08dad4ac23c8
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Dec 2022 21:28:22.7444
 (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: 9/MVrq2Unzk9fshhZsXjeFwDtc1TB0qxoD6/NQn/AVZRs8Vb5jxpzOTL7wknY2xBX5rhXLjdjKNaxyhAHyU/Jw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6339
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,
        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

On Fri, Dec 02, 2022 at 11:27:07AM -0800, Dan Williams wrote:
> 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.

Thanks for chiming in here Steven.  I should have pushed back on this.

> 
> 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.

Ok I did not have any of these in there originally and I will remove them now.

Thanks!
Ira