>+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
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
