[openib-general] [PATCH] separates QP0/1 interactions

[Sean Hefty]

>@@ -205,11 +203,12 @@
> 	memset(mad_agent_priv, 0, sizeof *mad_agent_priv);
> 	mad_agent_priv->reg_req = reg_req;
> 	mad_agent_priv->rmpp_version = rmpp_version;
>+	mad_agent_priv->qp_info = &port_priv->qp_info[qp_type];
> 	mad_agent_priv->agent.device = device;
> 	mad_agent_priv->agent.recv_handler = recv_handler;
> 	mad_agent_priv->agent.send_handler = send_handler;
> 	mad_agent_priv->agent.context = context;
>-	mad_agent_priv->agent.qp = port_priv->qp[qp_type];
>+	mad_agent_priv->agent.qp = port_priv->qp_info[qp_type].qp;

The use of qp_type as an index is incorrect.  Maybe we should change
ib_verbs.h to set IB_QPT_SMI to 0 and IB_QPT_GSI to 1, so that their types
match their QP numbers.

>+
&mad_agent_priv->qp_info->send_posted_mad_list);
>+		mad_agent_priv->qp_info->send_posted_mad_count++;
>+		spin_unlock_irqrestore(&mad_agent_priv->qp_info-
>>send_list_lock,
>+				       flags);
>
> 		ret = ib_post_send(mad_agent->qp, &wr, &bad_wr);

As a side note, it's my intention to optimize the send path to avoid copying
the work request in the case where the send queue is not full.

>-static int convert_qpnum(u32 qp_num)
>-{
>-	/*
>-	 * No redirection currently!!!

This call wasn't needed with these changes.  When we do get to redirection,
I think that we can handle it more efficiently by placing needed queues and
such off the mad_agent structure.

> 	/* Setup MAD receive work completion from "normal" work completion
*/
> 	recv->header.recv_wc.wc = wc;
>-	recv->header.recv_wc.mad_len = sizeof(struct ib_mad); /* Should this
>be based on wc->byte_len ? Also, RMPP !!! */
>+	recv->header.recv_wc.mad_len = sizeof(struct ib_mad); /* ignore GRH
>size */
> 	recv->header.recv_wc.recv_buf = &recv->header.recv_buf;

I think that we should be able to eliminate the copying of work completion
structures when processing receive completions.  Doing so would require
having separate CQs for the send and receive queues, which isn't necessarily
a bad idea.  We could give priority to receive completion processing.

>+		/* Release locking before callback... */
> 		/* Invoke receive callback */
> 		mad_agent->agent.recv_handler(&mad_agent->agent,
> 					      &recv->header.recv_wc);
> 	}
> ...
>+	spin_unlock_irqrestore(&qp_info->port_priv->reg_lock, flags);

I need to think about how to handle this more, but I don't think that we
want to hold any spin locks while invoking callbacks.  This will likely
require referencing counting on the mad agent.  I think we may be able to
conceptually follow the CQ/QP event handling code.

>-	spin_unlock_irqrestore(&port_priv->send_list_lock, flags);
>+	spin_unlock_irqrestore(&qp_info->send_list_lock, flags);
>+
>+	/* Synchronize with deregistration... */

Send completion callbacks were invoked without holding a lock.  Same comment
as above.

> 	while (1) {
> 		while (!signal_pending(current)) {
>-			ret =
wait_event_interruptible(mad_thread_priv->wait, 0);
>+			ret = wait_event_interruptible(qp_info->wait, 0);
> 			if (ret) {
> 				printk(KERN_ERR "ib_mad thread exiting\n");
> 				return 0;
> 			}
>
>-			ib_mad_completion_handler(port_priv);
>-
>+			ib_mad_completion_handler(qp_info);
> 		}

I wanted to come back to this.  Should a thread be able to just spin on the
inner loop?

>-	attr_mask = IB_QP_STATE | IB_QP_PKEY_INDEX | IB_QP_PORT |
IB_QP_QKEY;
>+	attr_mask = IB_QP_STATE | IB_QP_PKEY_INDEX | IB_QP_QKEY;

Oops, this should have been a separate patch.  Port numbers cannot be
modified for special QPs.  I can resubmit this separately.

>+	init_waitqueue_head(&qp_info->wait);
>+	qp_info->mad_thread = kthread_create(ib_mad_thread_handler,
>+					     qp_info,
>+					     "ib_mad-%-6s-%-2d-%-4d",
>+
qp_info->port_priv->device->name,
>+					     qp_info->port_priv->port_num,
>+					     qp_info->qp->qp_num);

Right now, this changes the threading to be one per QP.  (It was the easiest
change.)  We can discuss how we want the threading to be: one per
system/device/port/QP/CPU...

Also, we can have separate completion handlers for QP 0 and QP 1 if needed.
(I doubt it will be.)