Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command

[Ira Weiny]

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@xxxxxxxxx> 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