[PATCH 2/6] New Infiniband (OFED) tapset
Josh Stone
jistone@redhat.com
Fri Feb 6 09:11:00 GMT 2009
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
More information about the Systemtap
mailing list