Re: [PATCH v5 01/27] IB/Verbs: Implement new callback query_transport()

[Doug Ledford]

On Wed, 2015-04-22 at 15:21 +0000, Devesh Sharma wrote:
> > -----Original Message-----
> > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma-
> > owner@xxxxxxxxxxxxxxx] On Behalf Of Doug Ledford
> > Sent: Wednesday, April 22, 2015 8:33 PM
> > To: Michael Wang
> > Cc: Roland Dreier; Sean Hefty; linux-rdma@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; hal@xxxxxxxxxxxxxxxxxx; Tom Tucker; Steve Wise;
> > Hoang-Nam Nguyen; Christoph Raisch; Mike Marciniszyn; Eli Cohen; Faisal
> > Latif; Jack Morgenstein; Or Gerlitz; Haggai Eran; Ira Weiny; Tom Talpey; Jason
> > Gunthorpe
> > Subject: Re: [PATCH v5 01/27] IB/Verbs: Implement new callback
> > query_transport()
> > 
> > On Mon, 2015-04-20 at 10:32 +0200, Michael Wang wrote:
> > > Add new callback query_transport() and implement for each HW.
> > 
> > The more I think about it, the more I think we need to eliminate this patch
> > entirely.
> > 
> > The problem here is that, if we follow my suggestion, then we are going to
> > eliminate the query as an API function and replace the information it gives us
> > with a static port attribute bitmap.  If we do this patch, then reform this patch
> > to my idea later, we introduce a very short lived API/ABI change in the kernel
> > module interface that serves absolutely no purpose.  Instead, let's do the
> > bitmap creation first, update the drivers to properly set the bitmap, then do all
> > of the remaining reforms you have here using that bitmap and completely skip
> > the
> > query_transport() API item that will no longer serve a purpose.
> 
> Any vendor device that registers with IB stack already has capability flags, are you referring to the same as bit maps?
> Is it possible to use same as bitmaps you are referring to? I am trying to understand you complete idea.

There are two capability flags right now, a device caps flag set and a
port caps flag set (not counting possible driver internal flags).
Regardless of whether or not we wanted to use one or the other, they are
both too full to be used for our purposes.  We can't get enough bits.
We would get to drop the node_type, but that's only a u8 and so it
doesn't have enough bits either.  In addition, node_type is set per
device, and it would really be best if our new bitmap were per port.

That then raises the issue that right now, the core code doesn't have
direct access to per-port information, everything is done via a
combination of direct access to the per device node check and per port
driver callbacks.  We may have no choice but to continue with that for
now, but I find that inefficient.  And if we make the bitmap per port,
then even our node check becomes a callback.

So, what we need to do, is define a specific bitmap just for the
node/transport/link bits and the capability bits that explicitly go with
that tuple *and* define a way to access it without necessarily requiring
a callback.

Michael's patch set has gone through a lot of revisions and I think we
are getting close to the set of things we want to know, so it shouldn't
be that hard now to create the proper bitmap that makes knowing those
things quick and efficient.

The harder part will be making it accessible.  Since the current struct
ib_device doesn't include direct access to the ports (it has a list head
for port_list, but that's just for the sysfs entries for this device, it
doesn't hold anything we can use for this) we are limited to either A)
adding a new callback or B) changing node_type to port_type, making it a
larger size, and making it a port indexed array.

-- 
Doug Ledford <dledford@xxxxxxxxxx>
              GPG KeyID: 0E572FDD

Attachment: signature.asc
Description: This is a digitally signed message part