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

[Michael S. Tsirkin]

Quoting r. Sean Hefty <mshefty at ichips.intel.com>:
> Subject: Re: [PATCH] mad: add GID/class checking for matching received to sent MADs
> 
> Jack Morgenstein 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?

> >+	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.

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.

> Failing that, we should move the 
> query when sending a MAD, so it can be done once.

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

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies