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

[Steven Rostedt]

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.

-- Steve

> 
> > +				for (i = 0; i < nr_rec; i++)
> > +					trace_cxl_generic_event(dev_name(cxlds->dev),
> > +								type,
> > +								&payload->records[i]);
> > +			}
> > +
> > +			rc = cxl_clear_event_record(cxlds, type, payload, nr_rec);
> > +			if (rc) {
> > +				dev_err(cxlds->dev, "Event log '%s': Failed to clear events : %d",
> > +					cxl_event_log_type_str(type), rc);
> > +				return;
> > +			}
> >  		}
> >