[openib-general] [PATCH] [RMPP] receive RMPP support

[Sean Hefty]

>I'm not sure if/how RMPP will be used, so it's not clear to me how
>useful the RMPP functions are -- I don't feel qualified to have an
>opinion yet.

I wouldn't have generic functions be named or specific to RMPP.  In looking
at the CM, RMPP, SA query, SMI, and now ping code, there's a general
commonality between them for sending a MAD.  There's similarity in structure
definitions, along with the allocation and formatting of the send_wr.  My
thought is that if we can get an agreement on a send_mad structure,
additional support by the MAD layer could make sending MADs a little easier
without the need to change the existing APIs (for the most part, if at all).

>>+{
>>+	struct ib_ah_attr *ah_attr;
>>+	struct ib_ah *ah;
>>+
>>+	ah_attr = kmalloc(sizeof *ah_attr, GFP_KERNEL);
>
>Is it really worth kmalloc() for ah_attr here?  struct ib_ah_attr is
>only 32 bytes.  Between the ah_attr pointer and the stack used by the
>call to kmalloc(), the current code is probably using at least 16
>bytes.  I'd trade 16 bytes of stack for smaller source and object code.

I'll remove the kmalloc here before committing the patch.  Thanks.

>>+	if (!ah_attr)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	memset(ah_attr, 0, sizeof *ah_attr);
>>+	ah_attr->dlid = wc->slid;
>>+	ah_attr->sl = wc->sl;
>>+	ah_attr->src_path_bits = wc->dlid_path_bits;
>>+	ah_attr->port_num = port_num;
>
>What if the wc has IB_WC_GRH set?  I'm not sure how useful a helper
>this is if it doesn't handle the GRH case.

I considered GRH, and my thought was to update the call for GRH support when
it's needed.  I can go back and verify that the API takes all the required
information for GRH support.

>>+	msg->sge.addr = dma_map_single(mad_agent->device->dma_device,
>>+				       &msg->mad, sizeof msg->mad,
>>+				       DMA_TO_DEVICE);
>
>it's somewhat risky to use dma_map_single() on fields in the middle of
>a structure, because you don't know that the field starts and ends on
>a cacheline boundary.  In this case you're pretty safe because you're
>doing DMA_TO_DEVICE, but if you use the same type of code with
>DMA_FROM_DEVICE on a non-cache-coherent arch (e.g. PowerPC 4xx) then
>you can get into trouble.  See <http://lwn.net/Articles/2265/> for a
>very nice writeup of the problem.

Thanks for the link.  I'll look at updating this before committing the patch
as well.

- Sean