Re: [PATCH 2/6] New Infiniband (OFED) tapset

[Josh Stone <jistone at redhat dot 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