On Wed, 13 Feb 2019 12:55:25 +0000 "Ruhl, Michael J" <michael.j.ruhl@xxxxxxxxx> wrote: > >I agree. But Mike is implying that there is a way to have > >trace_create_mad_addr() called within the > >trace_ib_mad_ib_sned_mad_enabled() trace point. IMO, there is not much Yeah, it should be possible to just pass in qp_info as well to the tracepoint and do the processing in the TP__fast_assign() portion. > >difference where the helper gets called when enabled. (probably a bit more > >efficient within the trace point but likely not much). > > > >When the trace is disabled there is not a significant performance penalty. > >Therefore, what Mike is suggesting simply makes the code a bit cleaner. But I > >may not understand some magic within tracing. > > This is for performance and cleanup. If the _create_mad() function can be > incorpated in th enty portion of the trace, then the explicit if trace_enabled() > portion would go away. Note, the trace_enabled() is implemented with jump_labels, thus there is no compare operation in the assembly. It's a simple nop, that gets converted into a non-conditional jump when enabled. The performance added by removing the if trace_enabled() is extremely negligible. Unless of course it is run on an arch that doesn't support jump labels, but that would have other performance issues as well. > > I haven't looked at the assembly, but I think that the if (trace_enabled) will > actually be in the code path regardless of trace being enabled or not. It shouldn't be. The jump label if block is set as an unlikely, and gcc should know enough to move that code to the end of the function (out of the hot path). Again, the only thing that the assembly should see is a single nop. > With > just the trace_xxx() in the code path, it will be not be present unless trace is > enabled (i.e. no if check, no performance cost). Again, there is no "if check". > > >If we can agree we are not looking at a bad performance hit here. I propose > >we accept the patches as is and I will clean it up later. (Including making the > >function name more clear.) > > > >Any objections? > > Nope. I wasn't sure if added if statement had been done on purpose or not. :) > > If performance is not an issue for this path, a follow up patch is fine with me. -- Steve
