Re: device attr cleanup (was: Handle mlx4 max_sge_rd correctly)

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2741 bytes --]

On 12/11/2015 01:14 AM, Jason Gunthorpe wrote:
> On Thu, Dec 10, 2015 at 11:56:50PM -0500, Doug Ledford wrote:
> 
>> Looking at struct netdevice, it has the sort of organization I would
>> call reasonable.  Things like struct tx_stats is a struct, even though
>> it's embedded in the parent struct and not a pointer and there is
>> exactly and only one copy, so it could be flat.
> 
> struct net_device_stats exists because it is used in many more places
> that just in struct net_device, so this is a code sharing thing.

It doubles well as decent organization.

>> something I would call well organized.  And speaking of that, in struct
>> netdevice, all of the various ops elements are handled as structs,
> 
> .. and 'struct net_device_ops' is used extensively.
> 
> Whereas once we get rid of the query call ib_device_attr has no second
> user.
> 
>> including the base netdevice ops, whereas struct ib_device puts the base
>> ops flat in the struct.  So if we wanted to be more like struct
>> netdevice, we would move the base ops out of struct ib_device.
> 
> It would make the drivers a bit nicer if they initialized an ib_ops
> structure like netdev does instead of in code, but performance wise
> this is probably better, especially if we sorted the struct members
> sanely..

You could still create a struct and embed the struct in ibdev and it
would make reading ibdev *much* nicer while preserving this performance
benefit (which is probably very minor, if at all, and very well might
become a performance penalty if you have more than one ibdev in heavy
use on the system).  The huge list of ops in the middle of the struct is
a gigantic eyesore.

>> out like this.  An ib_device can be multi-port, and each port can be a
>> different RDMA device type.
> 
> I thought we figured out that wasn't actually allowed today when
> working on the caps rework?

We figured out that the core code couldn't currently do something like
iWARP and RoCE on the same port even if registered as two different
ports.  We do IB/RoCE on different ports because they both appear as an
IB device, but I think the ongoing namespace work shows that they are
diverging more over time and eventually may not work in this fashion
nearly as cleanly as they did to start with.

I also commented that it was entirely understandable for a person to
want to do the whole iWARP/RoCE on one port via two registrations thing
once both the soft-RoCE and soft-iWARP drivers were in the kernel.  As
such, I'd like to see us not make achieving this goal harder in case we
decide to pursue it later.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]