[openib-general] [PATCH] RMPP: add Dual-sided RMPP support

[Jack Morgenstein]

Sorry for the delay in responding.
It seems to me that there are some bugs here.  This is based solely on
code review, so I may have erred. See my comments below

On Saturday 01 July 2006 06:58, Sean Hefty wrote:
> ===================================================================
>
> +static void ack_ds_ack(struct ib_mad_agent_private *agent,
> +		       struct ib_mad_recv_wc *recv_wc)
> +{
....

> +	rmpp_mad->rmpp_hdr.seg_num = 0;
The seg_num value will be set to 1 within ib_post_send_mad/ib_send_rmpp_mad -- 
see line 869 of mad_rmpp.c:
	if (rmpp_mad->rmpp_hdr.rmpp_type != IB_MGMT_RMPP_TYPE_DATA) {
		mad_send_wr->seg_num = 1;
		return IB_RMPP_RESULT_INTERNAL;
	}

In this case, the rmpp_type is NOT DATA, but is ACK, so seg_num will be set to 
1.

> +	rmpp_mad->rmpp_hdr.paylen_newwin = cpu_to_be32(1);
> +
> +	ret = ib_post_send_mad(msg, NULL);
> +	if (ret) {
> +		ib_destroy_ah(msg->ah);
> +		ib_free_send_mad(msg);
> +	}
> +}
> +

>  static void process_rmpp_ack(struct ib_mad_agent_private *agent,
...
> +	if (!mad_send_wr) {
The "if" below will not be taken ever, since seg_num has been set to 1 in the 
DS Ack packet.
> +		if (!seg_num)
> +			process_ds_ack(agent, mad_recv_wc, newwin);
> +		goto out;	/* Unmatched or DS RMPP ACK */
> +	}
> +


(mad_send_wr->timeout) on sending side is ALWAYS non-zero (see function 
send_next_seg() in mad_rmpp.c, line 574):
	/* 2 seconds for an ACK until we can find the packet lifetime */
	timeout = mad_send_wr->send_buf.timeout_ms;
	if (!timeout || timeout > 2000)
		mad_send_wr->timeout = msecs_to_jiffies(2000);
Therefore, it seems to me that every RMPP transaction will be double-sided, 
since the sending side will send the ack_ds_ack below.
> +	if ((mad_send_wr->last_ack == mad_send_wr->send_buf.seg_count) &&
> +	    (mad_send_wr->timeout)) {
> +		spin_unlock_irqrestore(&agent->lock, flags);
> +		ack_ds_ack(agent, mad_recv_wc);
> +		return;		/* Repeated ACK for DS RMPP transaction */
> +	}
>

I also failed to see where the second side of the double-sided RMPP 
transaction starts up (sending side).

Jack