lkml.org 
[lkml]   [2022]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 03/11] cxl/mem: Implement Clear Event Records command
    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.

    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.

    \
     
     \ /
      Last update: 2022-12-02 20:27    [W:4.083 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site