Messages in this thread | | | Date | Fri, 2 Dec 2022 13:28:19 -0800 | From | Ira Weiny <> | Subject | Re: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command |
| |
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
| |