[rds-devel] comments on the send CQ completion handler

[Or Gerlitz]

Hi Olaf,

Looking on rds_ib_send_cq_comp_handler I see few issues, see my inline comments:

+void rds_ib_send_cq_comp_handler(struct ib_cq *cq, void *context)
+{
+	struct rds_connection *conn = context;
+	struct rds_ib_connection *ic = conn->c_transport_data;
+	struct ib_wc wc;
+	struct rds_ib_send_work *send;
+	u32 completed;
+	u32 oldest;
+	u32 i = 0;
+	int ret;
+
+	rdsdebug("cq %p conn %p\n", cq, conn);
+	rds_ib_stats_inc(s_ib_tx_cq_call);
+	ret = ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
+	if (ret) {
+		rdsdebug("ib_req_notify_cq send failed: %d\n", ret);
+	}
+
+	while (ib_poll_cq(cq, 1, &wc) > 0 ) {
+		rdsdebug("wc wr_id 0x%llx status %u byte_len %u imm_data %u\n",
+			 (unsigned long long)wc.wr_id, wc.status, wc.byte_len,
+			 be32_to_cpu(wc.imm_data));
+		rds_ib_stats_inc(s_ib_tx_cq_event);
+
+		if (wc.wr_id == ~0) {
+			if (ic->i_ack_queued + HZ/2 < jiffies)
+				rds_ib_stats_inc(s_ib_tx_stalled);
+			rds_ib_ack_send_complete(ic);
+			continue;
+		}
+
+		oldest = rds_ib_ring_oldest(&ic->i_send_ring);
+		send = &ic->i_sends[oldest];
+
+		completed = rds_ib_ring_completed(&ic->i_send_ring, wc.wr_id, oldest);
+
+		for (i = 0; i < completed; i++) {
+			if (wc.opcode == IB_WC_SEND) {
+				if (send->s_rm)
+					rds_ib_send_unmap_rm(ic, send);
+			}
+			else if (wc.opcode == IB_WC_RDMA_WRITE) {
+				if (send->s_op)
+					dma_unmap_sg(ic->i_cm_id->device->dma_device,
+						send->s_op->r_sg, send->s_op->r_nents,
+						DMA_TO_DEVICE);
+			}
+			else if (wc.opcode == IB_WC_RDMA_READ) {
+				if (send->s_op)
+					dma_unmap_sg(ic->i_cm_id->device->dma_device,
+						send->s_op->r_sg, send->s_op->r_nents,
+						DMA_FROM_DEVICE);
+			}

This wc actually represents $completed completions so the value of wc.opcode is only
relevant for the --last-- iteration, a quite easy fix for that would be to mark the
opcode at the send strucutre.

+
+			send->s_wr.num_sge = 1;
+			if (send->s_queued + HZ/2 < jiffies)
+				rds_ib_stats_inc(s_ib_tx_stalled);
+			if (++send == &ic->i_sends[ic->i_send_ring.w_nr])
+				send = ic->i_sends;
+		}
+		rds_ib_ring_free(&ic->i_send_ring, completed);
+
+		if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags))
+			queue_delayed_work(rds_wq, &conn->c_send_w, 0);
+
+		/* We expect errors as the qp is drained during shutdown */
+		if (wc.status != IB_WC_SUCCESS && rds_conn_up(conn)) {
+			rds_ib_conn_error(conn,
+				"completion on %u.%u.%u.%u "
+				"had status %u, disconnecting and reconnecting\n",
+				NIPQUAD(conn->c_faddr), wc.status);
+		}

if wc.status is not success then all the other fields (speficially opcode) are invalid
except for the wr_id, an easy fix would be to move that check to be following the call
to ib_poll_cq, agree?

+	}
+}

Or.