[openib-general] Re: [PATCH] mad: add GID/class checking for matching received to sent MADs

[Sean Hefty]

Michael S. Tsirkin wrote:
>>>+static inline int rcv_has_same_gid(struct ib_mad_send_wr_private *wr,
>>>+				   struct ib_mad_recv_wc *rwc )
>>>+{
>>>+	struct ib_ah_attr attr;
>>>+	u8 send_resp, rcv_resp;
>>>+
>>>+	send_resp = ((struct ib_mad *)(wr->send_buf.mad))->
>>>+		     mad_hdr.method & IB_MGMT_METHOD_RESP;
>>>+	rcv_resp = rwc->recv_buf.mad->mad_hdr.method & IB_MGMT_METHOD_RESP;
>>>+
>>>+	if (!send_resp && rcv_resp)
>>>+		/* is request/response. GID/LIDs are both local (same). */
>>>+		return 1;
>>>+
>>>+	if (send_resp == rcv_resp)
>>>+		/* both requests, or both responses. GIDs different */
>>>+		return 0;
>>
>>The two checks above are only checking response bits of a sent and received 
>>MAD. How do these relate to checking the GIDs?
> 
> 
> Well, the comments really explain what the checks do, don't they?
> Would rcv_has_same_initiator be a better name?

The comments don't help.  Returning that the GIDs match just because a send is a 
request and a receive is a response is misleading.  How does the function know this?

>>>+	if (ib_query_ah(wr->send_buf.ah, &attr))
>>>+		/* Assume not equal, to avoid false positives. */
>>>+		return 0;
>>
>>Querying for the address handle every time seems expensive.
> 
> 
> Its a very cheap operation. Look at the implementation: the low level
> driver already caches everything in hardware format, we just move
> some bits around.
> 
> 
>>Can't we save 
>>the necessary information inside the ah?
> 
> 
> That would waste a lot of memory.

If the driver is caching the memory anyway, couldn't it just store the data in 
the ah, rather than some private structure?

> Its not worth caching it.  Our locking rules require for query_ah to be atomic
> which is a strong hint that it just retrieves the data cached by low level
> driver.

If the locking rules require the data to be cached by the driver, why not make 
it available to the user directly?

> We do it on receive to match ACKs to transactions.
> I dont see how this check can be done on send.
> Patch?

I was suggesting that the query be done when sending the MAD and the results 
saved.  This makes less sense if the ah attributes need to be cached by the 
driver anyway.

- Sean