This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi -

David J. Wilder wrote:
This tapset is the first of several tapsets used to probe the ofed
infiniband stack.  Ofed.stp includes probes to several of the ib
"kernel" verbs.  Also included are core functions that users of the ofed
tapescripts may find helpful

I know little about infiniband, so I'm mostly peeking through here for safety issues and adherence to conventions...

+%{
+
+#define get_memberfault(STRUCT, MEMBER)				\
+	goto end;						\
+deref_fault:							\
+	printk("deref fault in ofed.stp, base pointer = %p\n",	\
+	(struct STRUCT *)THIS->A); 				\
+	THIS->__retvalue = -1;					\
+end: ;

The kread will prepare an error string for the normal stap error channels (c->last_error), so you don't need another message in printk. You can also use CATCH_DEREF_FAULT() to provide the deref_fault label, unless you really want the retvalue set to -1.

+#define __get_member(STRUCT,MEMBER) kread(&((struct STRUCT *)THIS->A)->MEMBER);

I think you need to cast with (struct STRUCT *)(long) for 32-bit platforms.


+#define	_get_member(STRUCT,MEMBER)	__get_member(STRUCT,MEMBER); get_memberfault(STRUCT,MEMBER);
+#define get_member(STRUCT,MEMBER,TYPE) (long)(TYPE)_get_member(STRUCT, MEMBER);
+%}

The kread will cast the value to typeof(STRUCT->MEMBER), so do you really need the manual TYPE cast?

+function ib_qp__device:long (A:long)
+ %{ THIS->__retvalue  =  get_member(ib_qp, device, void *)%}

Side-effect-free embedded-c like this should have a comment "/* pure */" so our optimizer knows that it may be possible to remove it.

I'm working on bz5634 to support native type casting, so hopefully
really soon it will be possible to write such functions as:

  function ip_qp__device:long (A:long)
  { return @cast(A, "struct ib_qp")->device }

Not yet though...

+/**
+ * sfunction hexdump - Print a buffer in hex.
+ *
+ * Given the address and the length of a buffer, print its contents in hex. + */
+function hexdump (buff:long, len:long) {
+ printf("%s\n",_hexdump(buff, len))
+}

I think a stap-native printf("%.*M\n", 2*len, buff) will work like this...



+probe ib_post_send = 	ib_ehca_post_send ?,
+			ib_mlx4_post_send ?,
+			ib_mthca_post_send ?,
+			ib_nes_post_send ? {}

Our convention is to use dots for separating probe namespaces, so this should be something like "ib.post_send". Then for the internal variants, prefix it with an underscore like "_ib.ehca_post_send".

+		ib_post_send_qp = $qp
+		ib_post_send_wr = $send_wr

Since you're already in an ib-specific probe, I think these variables should be named more simply, like "qp" and "wr".


Josh



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]