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

["Vishwanathapura, Niranjana" <niranjana.vishwanathapura-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

Hi,

I haven't gone through the patches in detail yet.
Jason already responded regarding using 'rdma netdev' for this use case.
I agree with it and adding couple points to that here.

On Thu, Mar 02, 2017 at 01:06:19PM -0700, Jason Gunthorpe wrote:
>On Thu, Mar 02, 2017 at 09:13:33PM +0200, Erez Shitrit wrote:
>
>> 3. We are using some functions that are not part of the ndo calls or
>> are used differently for example:
>
>What these patches do is not consistent with the rest of the stack.
>
>Niranjana choose to add control functions directly to 'rdma_netdev', I
>think that is a reasonable choice and you should do the same:
>
>+struct rdma_netdev {
>+	void *clnt_priv;
>+
>+	/* control functions */
>+	void (*set_id)(struct net_device *netdev, int id);
>+};
>
>Add ipoib_attach_mcast/etc
>
>Maybe propose to Niranjana that this should be a struct
>rdma_netdev_ops pointer.
>
>For everything else I would use ndo ops directly and do not involve
>the rdma side.
>

Many of the operations in ib_ipoib_accel_ops can use ndo calls or can be easily 
be adopted to use them.

>> I think that we can change the create_netdev api to be like is in the
>> VNIC code, but we would like to keep the rest of the functions in
>> our
>
>Of course you can, it is the same function!
>
>Both patchsets need to adopt the same scheme for dealing with 'priv'
>data in the netdev, and the same signature for the create_netdev()
>driver op. The vnic approach is reasonable here and provides a
>dedicated priv for the driver and a second priv for the management
>layer.
>
>You should start here:
>
>https://patchwork.kernel.org/patch/9587819/
>
>And propose any fundamental modifications to Niranjana.
>
>For instance, ipoib should define a header like 'opib_vnic.h' that
>specifies ipoib versions of opa_vnic_rdma_netdev, opa_vnic_priv,
>opa_vnic_dev_priv, and so on.
>
>> struct and suggest the VNIC driver will use it in order to get the
>> pointer of the create_netdev, We think that struct of pointers to
>> functions can be flexible to future needs with no need to consider the
>> ndo existence.
>
>I think that misses the point, there many ndo ops that are
>relavent only to the low level driver (eg ndo_select_queue,
>ndo_features_check, etc) and those should just be directly hooked by
>the low level driver without involvement from the ipoib layer.
>
>For instance instead of having ipoib ndo_start_xmit call a
>ops->start(), it is more consistent with netdev for the driver to
>provide ndo_start_xmit and call ipoib_get_ah() to get the path
>information.
>
>This broadly makes more sense because the driver might want to return
>NETDEV_TX_BUSY right away and doesn't need to be forced to do a
>mandatory AH lookup by the ipoib wrapper.
>
>In general wrapping the ndo calls in ipoib is probably not a great
>idea. (same comment applies to vnic)
>

Having OPA_VNIC call a function out of ib_ipoib_accel_ops is not right design 
at all.
'rdma netdev' is a generic netdev interface which can be used for multiple use 
cases.
Both OPA_VNIC and this should use 'rdma netdev' interface. OPA_VNIC is already 
doing it by defining RDMA_NETDEV_OPA_VNIC.
The control ops should be associated with the 'rdma netdev' and not the other 
way round.

You can put the control ops in the ipoib wrapper around 'rdma_netdev' (opa_vnic 
defines opa_vnic_rdma_netdev structure, ipoib can define a similar one) or 
directly under 'rdma_netdev' structure perhaps.
I am seeing in PATCH#26, mlx5 driver is accessing ipoib ULP's private data 
structure, which it shouldn't. VNIC using rdma netdev clearly distinguishes 
between client and device private data structures.

Niranjana

>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