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

[Sean Hefty]

>+static int data_offset(u8 mgmt_class)
>+{
>+	if (mgmt_class == IB_MGMT_CLASS_SUBN_ADM)
>+		return IB_MGMT_SA_HDR;
>+	else if ((mgmt_class >= IB_MGMT_CLASS_VENDOR_RANGE2_START) &&
>+		 (mgmt_class <= IB_MGMT_CLASS_VENDOR_RANGE2_END))
>+		return IB_MGMT_VENDOR_HDR;
>+	else
>+		return IB_MGMT_RMPP_HDR;
>+}

I think that the RMPP code may have this same routine.  If so, maybe we can make
this an inline function.

>+static int copy_recv_mad(struct ib_mad_recv_wc *mad_recv_wc,
>+			  struct ib_umad_packet *packet)
>+{
>+	struct ib_mad_recv_buf *seg_buf;
>+	struct ib_rmpp_mad *rmpp_mad;
>+	void *data;
>+	struct ib_mad_multipacket_seg *seg;
>+	int size, len, offset;
>+	u8 flags;
>+
>+	len = mad_recv_wc->mad_len;
>+	if (len <= sizeof(struct ib_mad)) {
>+		memcpy(&packet->mad.data, mad_recv_wc->recv_buf.mad, len);
>+		return 0;
>+	}
>+
>+	/* Multipacket (RMPP) MAD */
>+	offset = data_offset(mad_recv_wc->recv_buf.mad->mad_hdr.mgmt_class);
>+
>+	list_for_each_entry(seg_buf, &mad_recv_wc->rmpp_list, list) {
>+		rmpp_mad = (struct ib_rmpp_mad *)seg_buf->mad;
>+		flags = ib_get_rmpp_flags(&rmpp_mad->rmpp_hdr);
>+
>+		if (flags & IB_MGMT_RMPP_FLAG_FIRST) {
>+			size = sizeof(*rmpp_mad);
>+			memcpy(&packet->mad.data, rmpp_mad, size);
>+		} else {
>+			data = (void *) rmpp_mad + offset;
>+			if (flags & IB_MGMT_RMPP_FLAG_LAST)
>+				size = len;
>+			else
>+				size = sizeof(*rmpp_mad) - offset;
>+			seg = kmalloc(sizeof(struct ib_mad_multipacket_seg) +
>+				      sizeof(struct ib_rmpp_mad) - offset,
>+				      GFP_KERNEL);
>+			if (!seg)
>+				return -ENOMEM;
>+			memcpy(seg->data, data, size);
>+			list_add_tail(&seg->list, &packet->seg_list);
>+		}
>+		len -= size;
>+	}
>+	return 0;
>+}

It would be more efficient to just queue the received MAD until it can be copied
directly to the userspace buffer, rather than copying it into a temporary
buffer.

>+
>+static struct ib_umad_packet *alloc_packet(void)
>+{
>+	struct ib_umad_packet *packet;
>+	int length = sizeof *packet + sizeof(struct ib_mad);
>+
>+	packet = kzalloc(length, GFP_KERNEL);
>+	if (!packet) {
>+		printk(KERN_ERR "alloc_packet: mem alloc failed for length
%d\n",
>+		       length);
>+		return NULL;
>+	}
>+	INIT_LIST_HEAD(&packet->seg_list);
>+	return packet;
>+}

We should probably just drop this function.  It looks like it's only called in
one place, plus would only save a single line of code for each place that it is
called.

- Sean