Re: [dpdk-dev] discussion: creating a new class for vdpa drivers

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi Maxime,

On 12/16/19 11:50 AM, Maxime Coquelin wrote:
> Hi Andrew,
> 
> On 12/16/19 9:46 AM, Andrew Rybchenko wrote:
>> On 12/16/19 11:29 AM, Matan Azrad wrote:
>>>
>>> Hi all
>>>
>>> I understand all of you agree \ not object with the new class for vdpa drivers.
>>
>> I have two control questions:
>>
>> 1. If so, is it allowed to have vDPA driver in
>>    drivers/net/<driver> if it is better from code sharing point
>>    of view?
> 
> If it has something to share, I think we should move the common bits
> to the common directory.

Does it mean that it is *not* allowed to have vdpa driver in
drivers/net/<driver> and vDPA drivers *must* live in
drivers/vdpa only?

>> 2. If drivers/common is used, is exported functions which are
>>    used by drivers/net/<driver> and drivers/vdpa/<driver> and
>>    data structures are a part of public API/ABI? Hopefully not,
>>    but I'd like to double-check and ensure that it is solved in
>>    the case of shared libraries build.
> 
> Common functions and data should not be part of the API/ABI I agree.
> I guess we should use relative paths for including the common headers.

Hopefully include_directories() with relative path in the case
of meson.build.

>>> Based on that, I'm going to start it.
>>>
>>> From: Tiwei Bie
>>>> On Tue, Dec 10, 2019 at 09:00:33AM +0100, Thomas Monjalon wrote:
>>>>> 10/12/2019 03:41, Tiwei Bie:
>>>>>> On Mon, Dec 09, 2019 at 02:22:27PM +0300, Andrew Rybchenko wrote:
>>>>>>> On 12/9/19 1:41 PM, Ori Kam wrote:
>>>>>>>> From: Andrew Rybchenko
>>>>>>>>> On 12/8/19 10:06 AM, Matan Azrad wrote:
>>>>>>>>>> From: Andrew Rybchenko
>>>>>>>>>>> On 12/6/19 8:32 AM, Liang, Cunming wrote:
>>>>>>>>>>>> From: Bie, Tiwei <tiwei.bie@intel.com>
>>>>>>>>>>>>> On Thu, Dec 05, 2019 at 01:26:36PM +0000, Matan Azrad wrote:
>>>>>>>>>>>>>> Hi all
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> As described in RFC “[RFC] net: new vdpa PMD for Mellanox
>>>>>>>>>>>>>> devices”, a new vdpa drivers is going to be added for
>>>>>>>>>>>>>> Mellanox devices – mlx5_vdpa
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The only vdpa driver now is the IFC driver that is located
>>>>>>>>>>>>>> in net
>>>>>>>>> directory.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The IFC driver and the new mlx5_vdpa driver provide the
>>>>>>>>>>>>>> vdpa ops
>>>>>>>>> and
>>>>>>>>>>>>>> not the eth_dev ops.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> All the others drivers in net provide the eth-dev ops.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I suggest to create a new class for vdpa drivers, to move
>>>>>>>>>>>>>> IFC to this class and to add the mlx5_vdpa to this class too.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Later, all the new drivers that implements the vdpa ops
>>>>>>>>>>>>>> will be added to the vdpa class.
>>>>>>>>>>>>>
>>>>>>>>>>>>> +1. Sounds like a good idea to me.
>>>>>>>>>>>> +1
>>>>>>>>>>>
>>>>>>>>>>> vDPA drivers are vendor-specific and expected to talk to vendor
>>>> NIC. I.e.
>>>>>>>>>>> there are significant chances to share code with network drivers
>>>> (e.g.
>>>>>>>>> base
>>>>>>>>>>> driver). Should base driver be moved to drivers/common in
>>>>>>>>>>> this case or is
>>>>>>>>> it
>>>>>>>>>>> still allows to have vdpa driver in drivers/net together with ethdev
>>>> driver?
>>>>>>>>>>
>>>>>>>>>> Yes, I think this should be the method, shared code should be
>>>>>>>>>> moved to
>>>>>>>>> the drivers/common directory.
>>>>>>>>>> I think there is a precedence with shared code in common which
>>>>>>>>>> shares a
>>>>>>>>> vendor specific code between crypto and net.
>>>>>>>>>
>>>>>>>>> I see motivation behind driver/vdpa. However, vdpa and net
>>>>>>>>> drivers tightly related and I would prefer to avoid extra
>>>>>>>>> complexity here. Right now simplicity over-weights for me.
>>>>>>>>> No strong opinion on the topic, but it definitely requires
>>>>>>>>> better and more clear motivation why a new class should be
>>>>>>>>> introduced and existing drivers restructured.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Why do you think there is extra complexity?
>>>>>>>
>>>>>>> Even grep becomes a bit more complicated J
>>>>>>>
>>>>>>>> I think from design correctness it is more correct to create a dedicated
>>>> class for the following reasons:
>>>>>>>> 1. All of the classes implements a different set of ops. For
>>>>>>>> example the cryptodev has a defined set of ops, same goes for the
>>>> compress driver and the ethdev driver. Each ones of them has different ops.
>>>> Going by this definition since VDPA has a different set of ops, it makes sense
>>>> that it will be in a different class.
>>>>>>>>
>>>>>>>> 2. even that both drivers (eth/vdpa) handle traffic from the nic
>>>>>>>> most of the code is different (the difference is also dependent on the
>>>> manufacture) So even the shared code base is not large and can be shared
>>>> using the common directory. For example ifc doesn't have any shared code.
>>>>>>>>
>>>>>>>> What do you think?
>>>>>>>
>>>>>>> The true reason is: if the difference in ops implemented is a key
>>>>>>> difference which should enforce location in different directories.
>>>>>>> Or underlying device class is a key.
>>>>>>> Roughly:
>>>>>>>  - net driver is a control+data path
>>>>>>>  - vdpa driver is a control path only My fear is that control path
>>>>>>> will grow more and more (Rx mode, RSS, filters and so on)
>>>>>>
>>>>>> I think this is a reasonable concern.
>>>>>>
>>>>>> One thing needs to be clarified is that, the control path (ops) in
>>>>>> vdpa driver is something very different with the control path in net
>>>>>> driver. vdpa is very generic (or more precisely vhost-related),
>>>>>> instead of network related:
>>>>>>
>>>>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
>>>>>>
>>>> thub.com%2FDPDK%2Fdpdk%2Fblob%2Faef1d0733179%2Flib%2Flibrte_vhos
>>>> t%2F
>>>>>> rte_vdpa.h%23L40-
>>>> L78&amp;data=02%7C01%7Cmatan%40mellanox.com%7Cfac15
>>>>>>
>>>> 729a67c4c81ee7608d77d7434a2%7Ca652971c7d2e4d9ba6a4d149256f461b%7C
>>>> 0%7
>>>>>>
>>>> C0%7C637115810358231304&amp;sdata=%2BZa39vxadtKx5Ov7vmqcU3RuZhz
>>>> kOP9o
>>>>>> 8roEB0d5j6M%3D&amp;reserved=0
>>>>>>
>>>>>> It's built on top of vhost-user protocol, manipulates virtqueues,
>>>>>> virtio/vhost features, memory table, ...
>>>>>>
>>>>>> Technically, it's possible to have blk, crypto, ...
>>>>>> vdpa devices as well (we already have vhost-user-blk,
>>>>>> vhost-user-crypto, ...).
>>>>>>
>>>>>> But network specific features will come eventually, e.g. RSS. One
>>>>>> possible way to solve it is to define a generic event callback in
>>>>>> vdpa ops, and vdpa driver can request the corresponding info from
>>>>>> vhost based on the event received.
>>>>>>
>>>>>> Another thing needs to be clarified is that, the control path
>>>>>> supposed to be used by DPDK apps directly in vdpa should always be
>>>>>> generic, it should just be something like:
>>>>>>
>>>>>> int rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr); int
>>>>>> rte_vhost_driver_attach_vdpa_device(const char *path, int did); int
>>>>>> rte_vhost_driver_detach_vdpa_device(const char *path); ...
>>>>>>
>>>>>> That is to say, users just need to bind the vdpa device to the vhost
>>>>>> connection. The control path ops in vdpa is supposed to be called by
>>>>>> vhost-library transparently based on the events on the vhost-user
>>>>>> connection, i.e.
>>>>>> the vdpa device will be configured (including RSS) by the guest
>>>>>> driver in QEMU "directly" via the vhost-user protocol instead of the
>>>>>> DPDK app in the host.
>>>>>
>>>>> Tiwei, in order to be clear,
>>>>> You think vDPA drivers should be in drivers/vdpa directory?
>>>>
>>>> I was just trying to clarify two facts in vDPA to address Andrew's concern.
>>>> And back to the question, to make sure that we don't miss anything
>>>> important, (although maybe not very related) it might be helpful to also
>>>> clarify how to support vDPA in OvS at the same time which isn't quite clear to
>>>> me yet..
>>>>
>>>> Regards,
>>>> Tiwei
>>>>
>>>>>
>>>>>
>>
>