Re: [RFC for accelerated IPoIB 00/27] Enhanced mode for IPoIB driver

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

On Wed, Mar 01, 2017 at 04:02:08PM +0200, Erez Shitrit wrote:
>     /* default accelerating functions, same as before */
>     struct ib_ipoib_accel_ops default_ipoib_accel_ops = {
>             .ib_dev_init = ipoib_dev_init_default,
>             .ib_dev_cleanup = ipoib_dev_uninit_default,
>             .ib_dev_open = ipoib_ib_dev_open_default,
>             .ib_dev_stop = ipoib_ib_dev_stop_default,
>             .send = ipoib_send,
>             .create_netdev = ipoib_create_netdev_default,
>             .attach_mcast = ipoib_mcast_attach,
>             .dettach_mcast = ipoib_mcast_dettach,
>     };

This is so ridiculously close to what opa_vnic is doing, please try to
work with them to figure out some common version of your
'create_netdev':

+struct ib_ipoib_accel_ops {
+
+       /*
+        * HW provider driver creates the net_device for IPoIB.
+        * hca: The current ib device.
+        * name: is the format of the new network device (probably ib%d)
+        */
+       struct net_device * (*create_netdev)(struct ib_device *hca,
+                                            const char *name,
+                                            void (*setup)(struct net_device *));


vs:

@@ -2110,6 +2128,15 @@  struct ib_device {

+	struct net_device *(*alloc_rdma_netdev)(
+					struct ib_device *device,
+					u8 port_num,
+					enum rdma_netdev_t type,
+					const char *name,
+					unsigned char name_assign_type,
+					void (*setup)(struct net_device *));


I've pointed out again and again you two are working on the same
thing.

It is inexplicable to me you couldn't use this opa_vnic patch as a
starting point for the ipoib api:

https://patchwork.kernel.org/patch/9587819/

I would propose you add a RDMA_NETDEV_IPOIB to their stuff and a
rdma_netdev_get_ipoib_ops(struct net_device *) to get your
rdma_netdev_ipoib_ops.

I also wonder why there are so many ops, for instance the approach of
having the driver set .ndo_stop and others seems better.

ipoib could either 'wrapper' that pointer after calling
alloc_rdma_netdev or it could export enough utility functions for the
driver to implement the needed behavior. (eg this may be more in line
with the "midlayer mistake" thinking of
https://lwn.net/Articles/336262/)

Maybe Niranjana has some suggestions based on learning in vnic, they
tried a few different schemes.

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