>
> 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.
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?
Ira
