>-----Original Message-----
>From: Weiny, Ira
>Sent: Wednesday, February 13, 2019 12:02 AM
>To: Steven Rostedt <rostedt@xxxxxxxxxxx>
>Cc: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx;
>Hal Rosenstock <hal@xxxxxxxxxxxxxxxxxx>; Doug Ledford
><dledford@xxxxxxxxxx>; Jason Gunthorpe <jgg@xxxxxxxx>; Ingo Molnar
><mingo@xxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel
>Borkmann <daniel@xxxxxxxxxxxxx>; Leon Romanovsky <leon@xxxxxxxxxx>
>Subject: RE: [PATCH for-next V4 1/6] IB/MAD: Add send path trace points
>
>>
>> On Tue, 12 Feb 2019 15:33:07 -0800
>> Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
>>
>> > > >+#define CREATE_TRACE_POINTS
>> > > >+#include <trace/events/ib_mad.h>
>> > > >+
>> > > > static int mad_sendq_size = IB_MAD_QP_SEND_SIZE; static int
>> > > > mad_recvq_size = IB_MAD_QP_RECV_SIZE;
>> > > >
>> > > >@@ -1223,6 +1268,14 @@ int ib_send_mad(struct
>> > > >ib_mad_send_wr_private
>> > > >*mad_send_wr)
>> > > >
>> > > > spin_lock_irqsave(&qp_info->send_queue.lock, flags);
>> > > > if (qp_info->send_queue.count < qp_info->send_queue.max_active)
>{
>> > > >+ if (trace_ib_mad_ib_send_mad_enabled()) {
>> > > >+ struct rdma_mad_trace_addr addr;
>> > > >+
>> > > >+ trace_create_mad_addr(qp_info->port_priv-
>>device,
>> > > >+ qp_info->port_priv-
>>port_num,
>> > > >+ &mad_send_wr->send_wr,
>> > > >&addr);
>> > > >+ trace_ib_mad_ib_send_mad(mad_send_wr,
>&addr);
>> > > >+ }
>> > > > ret = ib_post_send(mad_agent->qp, &mad_send_wr-
>> > > >>send_wr.wr,
>> > > > NULL);
>> > > > list = &qp_info->send_queue.list;
>> > >
>> > > Ira,
>> > >
>> > > Are the trace_ib_mad_ib_send_mad_enabled() (and the other
>locations)
>> > > actually necessary?
>> > >
>> > > When you populate the TP_STRUCT_entry values, you can call a
>> > > function to get information (i.e. see the hfi1/trac_ibhdrs.h usage of
>> hfi1_trace_packet_hdr_len().
>> > >
>> > > You may be able to call the trace_create_mad_addr() there (will
>> > > probably need to pass qp_info into trac_ib_mad_ib_send_mad()) rather
>> > > than having to have this enable check in the code.
>> >
>> >
>> > How much of an optimization is this? Is it more for code cleanliness?
>> > I don't see how there would be much performance difference here.
>> >
>> > If it is for code cleanliness will you accept me cleaning it up later?
>>
>> It looks like that "trace_create_mad_addr()" is not a trace event (I would
>> recommend renaming it to create_trace_mad_addr() to avoid the
>confusion).
>> That means it's a real function call that will get called and perform work all
>> the time, and we don't want that unless the ip_mad_ip_send_mad trace
>> event is enabled. Which shows that the
>> trace_ib_mad_ib_send_mad_enabled() check makes a significant
>difference.
>
>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
>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.
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. 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).
>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.
Mike
>Ira
