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.
-- Steve
