[openib-general] RE: [PATCH 3 of 3] mad: large RMPP support

[Sean Hefty]

>-static inline u64 get_seg_addr(struct ib_mad_send_wr_private *mad_send_wr)
>+static inline void *get_seg_addr(struct ib_mad_send_wr_private *mad_send_wr)
> {
>-	return mad_send_wr->sg_list[0].addr + mad_send_wr->data_offset +
>-	       (sizeof(struct ib_rmpp_mad) - mad_send_wr->data_offset) *
>-	       (mad_send_wr->seg_num - 1);
>+	struct ib_mad_multipacket_seg *seg;
>+	int i = 2;
>+
>+	list_for_each_entry(seg, &mad_send_wr->multipacket_list, list) {
>+		if (i == mad_send_wr->seg_num)
>+			return seg->data;
>+		i++;
>+	}
>+	return NULL;
> }

It would be more efficient if the payload segments were stored in an array.
We're going to end up walking through the list N times in order to send N
segments.


>+
> struct ib_mad_send_buf * ib_create_send_mad(struct ib_mad_agent *mad_agent,
> 					    u32 remote_qpn, u16 pkey_index,
> 					    int rmpp_active,
>@@ -787,39 +798,38 @@ struct ib_mad_send_buf * ib_create_send_
> {
> 	struct ib_mad_agent_private *mad_agent_priv;
> 	struct ib_mad_send_wr_private *mad_send_wr;
>-	int length, buf_size;
>+	int length, message_size, seg_size;
> 	void *buf;
>
> 	mad_agent_priv = container_of(mad_agent, struct ib_mad_agent_private,
> 				      agent);
>-	buf_size = get_buf_length(hdr_len, data_len);
>+	message_size = get_buf_length(hdr_len, data_len);
>
> 	if ((!mad_agent->rmpp_version &&
>-	     (rmpp_active || buf_size > sizeof(struct ib_mad))) ||
>-	    (!rmpp_active && buf_size > sizeof(struct ib_mad)))
>+	     (rmpp_active || message_size > sizeof(struct ib_mad))) ||
>+	    (!rmpp_active && message_size > sizeof(struct ib_mad)))
> 		return ERR_PTR(-EINVAL);
>
>-	length = sizeof *mad_send_wr + buf_size;
>-	if (length >= PAGE_SIZE)
>-		buf = (void *)__get_free_pages(gfp_mask,
>long_log2(roundup_pow_of_two(length)) - PAGE_SHIFT);
>-	else
>-		buf = kmalloc(length, gfp_mask);
>+	length = sizeof *mad_send_wr + message_size;
>+	buf = kzalloc(sizeof *mad_send_wr + sizeof(struct ib_mad), gfp_mask);
>
> 	if (!buf)
> 		return ERR_PTR(-ENOMEM);
>
>-	memset(buf, 0, length);
>-
>-	mad_send_wr = buf + buf_size;
>+	mad_send_wr = buf + sizeof(struct ib_mad);
>+	INIT_LIST_HEAD(&mad_send_wr->multipacket_list);
> 	mad_send_wr->send_buf.mad = buf;
>+	mad_send_wr->send_buf.mad_payload = buf + hdr_len;
>
> 	mad_send_wr->mad_agent_priv = mad_agent_priv;
>-	mad_send_wr->sg_list[0].length = buf_size;
>+	mad_send_wr->sg_list[0].length = hdr_len;
> 	mad_send_wr->sg_list[0].lkey = mad_agent->mr->lkey;
>+	mad_send_wr->sg_list[1].length = sizeof(struct ib_mad) - hdr_len;
>+	mad_send_wr->sg_list[1].lkey = mad_agent->mr->lkey;

The common case will be for single segment MADs.  We should try to pass a single
SGE to the hardware in this case, rather than requiring the hardware to fetch
two entries.


>
> 	mad_send_wr->send_wr.wr_id = (unsigned long) mad_send_wr;
> 	mad_send_wr->send_wr.sg_list = mad_send_wr->sg_list;
>-	mad_send_wr->send_wr.num_sge = 1;
>+	mad_send_wr->send_wr.num_sge = 2;
> 	mad_send_wr->send_wr.opcode = IB_WR_SEND;
> 	mad_send_wr->send_wr.send_flags = IB_SEND_SIGNALED;
> 	mad_send_wr->send_wr.wr.ud.remote_qpn = remote_qpn;
>@@ -827,6 +837,7 @@ struct ib_mad_send_buf * ib_create_send_
> 	mad_send_wr->send_wr.wr.ud.pkey_index = pkey_index;
>
> 	if (rmpp_active) {
>+		struct ib_mad_multipacket_seg *seg;
> 		struct ib_rmpp_mad *rmpp_mad = mad_send_wr->send_buf.mad;
> 		rmpp_mad->rmpp_hdr.paylen_newwin = cpu_to_be32(hdr_len -
> 						   IB_MGMT_RMPP_HDR + data_len);
>@@ -834,6 +845,27 @@ struct ib_mad_send_buf * ib_create_send_
> 		rmpp_mad->rmpp_hdr.rmpp_type = IB_MGMT_RMPP_TYPE_DATA;
> 		ib_set_rmpp_flags(&rmpp_mad->rmpp_hdr,
> 				  IB_MGMT_RMPP_FLAG_ACTIVE);
>+		mad_send_wr->total_length = message_size;
>+		/* allocate RMPP buffers */
>+		message_size -= sizeof(struct ib_mad);
>+		seg_size = sizeof(struct ib_mad) - hdr_len;
>+		while (message_size > 0) {
>+			seg = kmalloc(sizeof(struct ib_mad_multipacket_seg) +
>+				      seg_size, gfp_mask);
>+			if (!seg) {
>+				printk(KERN_ERR "ib_create_send_mad: RMPP mem "
>+				       "alloc failed for len %zd, gfp %#x\n",
>+				       sizeof(struct ib_mad_multipacket_seg) +
>+				       seg_size, gfp_mask);
>+				free_send_multipacket_list(mad_send_wr);
>+				kfree(buf);
>+				return ERR_PTR(-ENOMEM);
>+			}
>+			seg->size = seg_size;
>+			list_add_tail(&seg->list,
>+				      &mad_send_wr->multipacket_list);
>+			message_size -= seg_size;
>+		}
> 	}
>
> 	mad_send_wr->send_buf.mad_agent = mad_agent;
>@@ -842,23 +874,36 @@ struct ib_mad_send_buf * ib_create_send_
> }
> EXPORT_SYMBOL(ib_create_send_mad);


This function is getting fairly long.  Can we split it up?


>+struct ib_mad_multipacket_seg *ib_get_multipacket_seg(struct ib_mad_send_buf *
>+						      send_buf, int seg_num)
>+{
>+	struct ib_mad_send_wr_private *mad_send_wr;
>+	struct ib_mad_multipacket_seg *seg;
>+	int i = 2;
>+
>+	mad_send_wr = container_of(send_buf, struct ib_mad_send_wr_private,
>+				   send_buf);
>+	list_for_each_entry(seg, &mad_send_wr->multipacket_list, list) {
>+		if (i == seg_num)
>+			return seg;
>+		i++;
>+	}
>+	return NULL;
>+}
>+EXPORT_SYMBOL(ib_get_multipacket_seg);

Same list walking issue.

- Sean