Messages in this thread | | | Date | Fri, 2 Dec 2022 08:34:48 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command |
| |
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.
-- 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; > > + } > > } > >
| |