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

[Sean Hefty]

>+static inline void adjust_last_ack(struct ib_mad_send_wr_private *wr)
>+{
>+	struct ib_mad_multipacket_seg *seg;
>+
>+	if (wr->last_ack < 2)
>+		return;
>+	else if (!wr->last_ack_seg)
>+		list_for_each_entry(seg, &wr->multipacket_list, list) {
>+			if (wr->last_ack == seg->num) {
>+				wr->last_ack_seg = seg;
>+				break;
>+			}
>+		}
>+	else
>+		list_for_each_entry(seg, &wr->last_ack_seg->list, list) {
>+			if (wr->last_ack == seg->num) {
>+				wr->last_ack_seg = seg;
>+				break;
>+			}
>+		}
>+}

If we initialize last_ack_seg to the start of the list, can we combine the else
if and else checks together?

>@@ -647,6 +672,7 @@ static void process_rmpp_ack(struct ib_m
>
> 	if (seg_num > mad_send_wr->last_ack) {
> 		mad_send_wr->last_ack = seg_num;
>+		adjust_last_ack(mad_send_wr);

If last_ack_seg references a segment that contains the seg_num, can we eliminate
last_ack?

>+static inline int alloc_send_rmpp_segs(struct ib_mad_send_wr_private *send_wr,
>+				       int message_size, int hdr_len,
>+				       int data_len, u8 rmpp_version,
>+				       gfp_t gfp_mask)
>+{
>+	struct ib_mad_multipacket_seg *seg;
>+	struct ib_rmpp_mad *rmpp_mad = send_wr->send_buf.mad;
>+	int seg_size, i = 2;
>+
>+	rmpp_mad->rmpp_hdr.paylen_newwin =
>+			cpu_to_be32(hdr_len - IB_MGMT_RMPP_HDR + data_len);
>+	rmpp_mad->rmpp_hdr.rmpp_version = rmpp_version;
>+	rmpp_mad->rmpp_hdr.rmpp_type = IB_MGMT_RMPP_TYPE_DATA;
>+	ib_set_rmpp_flags(&rmpp_mad->rmpp_hdr, IB_MGMT_RMPP_FLAG_ACTIVE);
>+	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);

It would be convenient if the MAD were cleared for the user, so we don't end of
transferring random data, especially at the end of the user data.

>+		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(send_wr);
>+			return -ENOMEM;
>+		}
>+		seg->size = seg_size;

Okay, seg_size is the same for all segments belonging to a single MAD.  We can
move this it ib_mad_send_buf.

>+	mad_send_wr->last_ack_seg = NULL;
>+	mad_send_wr->seg_num_seg = NULL;

Mad_send_wr is cleared on allocation.  Are there better values to initialize
these variables to?  The first/second segment?

>+struct ib_mad_multipacket_seg
>+*ib_rmpp_get_multipacket_seg(struct ib_mad_send_wr_private *wr, int seg_num)
>+{
>+	struct ib_mad_multipacket_seg *seg;
>+
>+	if (seg_num == 2) {
>+		wr->seg_num_seg =
>+			container_of(wr->multipacket_list.next,
>+				     struct ib_mad_multipacket_seg, list);
>+		return wr->seg_num_seg;
>+	}
>+
>+	/* get first list entry if was not already done */
>+	if (!wr->seg_num_seg)

See previous comment.

>+struct ib_mad_multipacket_seg
>+*ib_mad_get_multipacket_seg(struct ib_mad_send_buf *send_buf, int seg_num)
>+{
>+	struct ib_mad_send_wr_private *wr;
>+
>+	if (seg_num < 2)
>+		return NULL;
>+
>+	wr = container_of(send_buf, struct ib_mad_send_wr_private, send_buf);
>+	return ib_rmpp_get_multipacket_seg(wr, seg_num);
>+}

Treating the first segment special seems somewhat confusing.  (Maybe this is a
result of how the MAD/RMPP header is copied down from userspace?)


>+	if (!rmpp_active && length > sizeof(struct ib_mad)) {
>+		ret = -EINVAL;
>+		goto err_ah;
>+	}
>+
> 	packet->msg = ib_create_send_mad(agent,
> 					 be32_to_cpu(packet->mad.hdr.qpn),
> 					 0, rmpp_active,

I think that ib_create_send_mad performs the same check.

- Sean