[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
> 
> 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?

We are comparing the GIDs of the requestor, remember?
I initiated one of the MADs and remote side initiated the other one
so I know they cant match.

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

No, because its keeping it in hardware format.

> >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?

They dont actually require it, see. But thats what drivers happen to do.

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

I dont think this can work.

> This makes less sense if the ah attributes need to be 
> cached by the driver anyway.

Right.

-- 
Michael S. Tsirkin
Staff Engineer, Mellanox Technologies