Re: [PATCH libibverbs v6 3/7] livibverbs: Add support for XRC SRQs

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Tue, Jun 04, 2013 at 09:44:24PM +0000, Hefty, Sean wrote:
> > >  enum verbs_context_mask {
> > >  	VERBS_CONTEXT_XRCD	= 1 << 0,
> > > -	VERBS_CONTEXT_RESERVED	= 1 << 1
> > > +	VERBS_CONTEXT_SRQ	= 1 << 1,
> > > +	VERBS_CONTEXT_RESERVED	= 1 << 2
> > >  };
> > 
> > Why is _RESERVED being re-numbered here? That worries me..
> > 
> > For that matter, what is it for?
> 
> I called it reserved, you can call it max.  It's to let libibverbs guard against vendor libraries built against future versions of the library, but run against an older one, using something like this:
> 
> 	if (mask >= VERBS_CONTEXT_RESERVED)
> 		abort...

Hurm, doesn't sound great, all this complexity is trying to avoid
aborts.

Better to have the driver set the things it supports during init, and
the have verbs mask off the things it doesn't support, so that the
result is what both verbs and the driver supports.

If the driver has a problem with missing verbs support then it should
deal with it internally. I expect in most cases it isn't an issue
since this is just structure over-allocation..

> > Frankly, I would ditch mask for verbs_context op members and other
> > structures that are size based. What does it mean if the size includes
> > get_srq_num, but VERBS_CONTEXT_SRQ is 0 and *get_srq_num is
> > null/non-null?
> 
> These mask values indicate to libibverbs that it can cast from
> struct ibv_* to struct verbs_*.

Okay, that make sense, but verbs_xrcd and verbs_srq don't have an ibv
counterpart, so these constants are never used?

A comment would help, and 'verbs_context_mask' is a poor name. It
looks like the usual comp_mask stuff applied to the structure it is
in.

verbs_supported_extended or something??

> > > @@ -103,4 +103,6 @@ IBVERBS_1.1 {
> > >
> > >  		ibv_cmd_open_xrcd;
> > >  		ibv_cmd_close_xrcd;
> > > +		ibv_cmd_create_srq_ex;
> > > +
> > >  } IBVERBS_1.0;
> > 
> > Hum, so drivers that implement XRC will need to be paired with a new
> > verbs... Bit disappointing, did we want to try and address that?
> 
> The extensions were added specifically to handle XRC.

At one point there was a desire to have the drivers able to work with
a range of ibverbs, if the driver links to these symbols then it will
only dynamic link with the latest ibverbs.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html