This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH 2/6] New Infiniband (OFED) tapset
- From: Josh Stone <jistone at redhat dot com>
- To: "David J. Wilder" <dwilder at us dot ibm dot com>
- Cc: systemtap at sourceware dot org, xma at us dot ibm dot com, pradeep at us dot ibm dot com, prasad at linux dot vnet dot ibm dot com
- Date: Thu, 05 Feb 2009 19:41:54 -0800
- Subject: Re: [PATCH 2/6] New Infiniband (OFED) tapset
- References: <1233880157.23376.36.camel@wilder.ibm.com>
Continuing my review...
David J. Wilder wrote:
This tapset is used to probe the connection manager layer of the ofed
infiniband stack (ib_cm.ko). Probe point are included to monitor calls
to the connection manager services and call-backs from the ib mad layer.
+function ib_cm_id__cm_handler:long (A:long)
+%{ THIS->__retvalue = get_member(ib_cm_id, cm_handler, void *) %}
The comments I gave in part one apply here too. Also, since these are
in a different file, it's not guaranteed that get_member will be
available here. Even if they're both included, they may be in inverted
order.
It may be better to make your get_member() generally available in
loc2c-runtime.h instead. But like I mentioned before, I'm hoping that
@cast() support will make this unnecessary...
+/* returns a pointer to the ib_cm_event */
+function cm_work__cm_event:long (P:long)
+%{
+ struct ib_cm_event *cmevent;
+ struct cm_work *work = (struct cm_work *)THIS->P;
You need a (long) for 32-bit platforms.
+ cmevent = (struct ib_cm_event *) &(work->cm_event);
This shouldn't need a cast at all.
+function get_mad:long (mad_rec_wc:long)
+%{
+ struct ib_mad_recv_wc *wc = (struct ib_mad_recv_wc *) THIS->mad_rec_wc;
+ THIS->__retvalue = (long)wc->recv_buf.mad;
Needs a kread().
+function ib_get_cm_event_from_mad:long (mad:long)
+%{
+ int event;
+ struct ib_mad *mad = (struct ib_mad *) THIS->mad;
+
+ switch (mad->mad_hdr.attr_id) {
Needs a kread().
+function cm_rej_reason:long (mad:long)
+%{
+ struct cm_rej_msg {
+ struct ib_mad_hdr hdr;
+ __be32 local_comm_id;
+ __be32 remote_comm_id;
+ /* message REJected:2, rsvd:6 */
+ u8 offset8;
+ /* reject info length:7, rsvd:1. */
+ u8 offset9;
+ __be16 reason;
+ u8 ari[IB_CM_REJ_ARI_LENGTH];
+ u8 private_data[IB_CM_REJ_PRIVATE_DATA_SIZE];
+ } __attribute__ ((packed)) *rej;
Ick. I understand why this is here, but I hope this structure never
changes...
+ rej = (struct cm_rej_msg *)THIS->mad;
+ THIS->__retvalue = (long)__be16_to_cpu(rej->reason);
Needs a kread().
+function dev_str_from_cm_id:long (cmid:long)
+%{
+ struct ib_cm_id *cm_id = (struct ib_cm_id *) THIS->cmid;
+ struct ipoib_cm_tx *tx = cm_id->context;
+ struct ipoib_dev_priv *priv = netdev_priv(tx->dev);
+ struct net_device *dev = priv->dev;
+ THIS->__retvalue = (long) &(dev->name);
Needs a few kread()s.
+probe ib_cm.cm_send_handler = module("ib_cm").function("cm_send_handler")
+{
+ ib_cm_dprint (proper_name(""), //name:string,
+ " ", //device:string,
+ ib_wc_status_num2str($mad_send_wc->status),//event:string,
+ $mad_send_wc->send_buf->context[0],//cm_id:long,
+ $mad_send_wc->send_buf->context[1],//cm_state:long,
+ 0, //qpn:long,
+ 0 //cm_rej_reason:long
+ );
+}
If I haven't enabled your dprints, then this doesn't give me any other
context. Can you save some of the interesting data in local variables?
I looked at part 3 as well, but didn't have much to add beyond what the
sort of things I already pointed out.
Josh