Re: [dpdk-dev] [RFC PATCH 0/4 v2] Extending DPDK with multiple device support

[Marc Sune <marc.sune@bisdn.de>]

On 13/04/15 21:44, Keith Wiles wrote:
> Hi All,
>
> Here is a set of RFC patches to update DPDK to support a generic set of
> devices. The patches that follow create two files eal_common_device.h
> and rte_common_device.c and then a few items in rte_ethdev.[ch] were moved
> to cleanup those files by moving the device generic values and functions
> into a common set of device generic files.

(Old thread. But in the context of some private pktdev discussions, I 
was asked to give some feedback on this specific version)

>
> In the rte_common_device.h file are a couple of macros to abstract a few
> common items from rte_ethdev.h which are not ethernet specific. These
> generic device specific structure members are place into macros to be
> placed at the top of each new device type crypto, hardware offload, dpi,
> compression and possible others to be defined later. In version 2 I used
> nested structures a bit cleaner from the macro design, but modified a lot
> more files.

I don't see easy and that really pays off to have a common rte_dev_data 
structure. So far the common data, as far as I can see, is name and 
_maybe_ MTU. The queues and number of queues is something difficult to 
abstract (e.g. KNI or rte_ring, 1 TX and 1 RX queue?), although it can 
be done.

>
> Most of the changes are to update the code to locate the new location of
> the members in the nested structures. To not try and rewrite code I
> used macros to help hide the changes, but these constructs are now used
> in a lot of files withint DPDK.
>
> I did not pull the Rx/Tx Routines into a common function, but it could be
> done if everyone agrees. It does not mean every device will use a common
> Rx/Tx routine. Without pulling Rx/Tx routines into a common file allows
> the device writer to have similar or different set of routines.

Actually, to me this is the part that makes more sense to abstract, 
because all the DPDK applications that have to deal with  RX/TX over 
heterogeneous devices have to construct the same de-multiplexing logic 
(create an abstraction of a generic port on top of rte libraries) that 
we could provide.

I think this patch set addresses more of a fundamental re-organization 
of data structures to maximize reusage than this "higher level" 
abstraction. I am commenting some stuff inline, to the point that I could.

>
> These patches do not contain any new devices as these are being work on
> today. I could have included the DPI (dpidev) routines we did in the PoC,
> but the DPI is not open source.
>
> More cleanup of ethdev could be done to remove NIC specific features not
> supported in all devices and someone needs to do that cleanup IMO.
>
> The code is untested and I wanted to get something our for others to poke
> at today and more could be pulled out of ethdev as well. I/We will be
> looking at testing the code as we get more completed.
>
> I have not finished up the crypto APIs yet, but I am planning to work on
> those additions today. The crypto code we are using is the Quick Assist
> code found on 01.org, but we need to update the code to be move DPDK
> friendly.
>
> The QAT code does have a modified like API similar to Linux Kernel crypto
> API and I want to review that code first.

Seeing the balance of additions/deletions doesn't look like much of a 
gain in terms of number of lines too.

Marc

>
> Regards,
> ++Keith
>
>
> Keith Wiles (4):
>    Adding the common device files for multiple device support
>    Add the ethdev changes for multiple device support
>    Add the test file changes for common device support
>    Update PMD files for new common device support
>
>   app/test-pmd/config.c                             |   6 +-
>   app/test-pmd/testpmd.h                            |   4 +-
>   app/test/test_kni.c                               |  12 +-
>   app/test/test_link_bonding.c                      |  24 +-
>   app/test/virtual_pmd.c                            | 106 +--
>   examples/link_status_interrupt/main.c             |   6 +-
>   lib/librte_eal/bsdapp/eal/Makefile                |   1 +
>   lib/librte_eal/common/Makefile                    |   1 +
>   lib/librte_eal/common/eal_common_device.c         | 185 +++++
>   lib/librte_eal/common/include/rte_common_device.h | 674 +++++++++++++++
>   lib/librte_eal/common/include/rte_log.h           |   1 +
>   lib/librte_eal/linuxapp/eal/Makefile              |   1 +
>   lib/librte_ether/rte_ethdev.c                     | 944 +++++++++-------------
>   lib/librte_ether/rte_ethdev.h                     | 340 ++------
>   lib/librte_kni/rte_kni.c                          |   4 +-
>   lib/librte_pmd_af_packet/rte_eth_af_packet.c      |  38 +-
>   lib/librte_pmd_bond/rte_eth_bond_8023ad.c         |  18 +-
>   lib/librte_pmd_bond/rte_eth_bond_alb.c            |  10 +-
>   lib/librte_pmd_bond/rte_eth_bond_api.c            | 142 ++--
>   lib/librte_pmd_bond/rte_eth_bond_args.c           |   2 +-
>   lib/librte_pmd_bond/rte_eth_bond_pmd.c            | 164 ++--
>   lib/librte_pmd_bond/rte_eth_bond_private.h        |   2 +-
>   lib/librte_pmd_e1000/em_ethdev.c                  | 156 ++--
>   lib/librte_pmd_e1000/em_rxtx.c                    |  94 +--
>   lib/librte_pmd_e1000/igb_ethdev.c                 | 302 +++----
>   lib/librte_pmd_e1000/igb_pf.c                     |  86 +-
>   lib/librte_pmd_e1000/igb_rxtx.c                   | 168 ++--
>   lib/librte_pmd_enic/enic.h                        |   4 +-
>   lib/librte_pmd_enic/enic_ethdev.c                 | 140 ++--
>   lib/librte_pmd_enic/enic_main.c                   |  30 +-
>   lib/librte_pmd_fm10k/fm10k_ethdev.c               | 154 ++--
>   lib/librte_pmd_fm10k/fm10k_rxtx.c                 |   4 +-
>   lib/librte_pmd_i40e/i40e_ethdev.c                 | 172 ++--
>   lib/librte_pmd_i40e/i40e_ethdev_vf.c              | 194 ++---
>   lib/librte_pmd_i40e/i40e_fdir.c                   |  28 +-
>   lib/librte_pmd_i40e/i40e_pf.c                     |   8 +-
>   lib/librte_pmd_i40e/i40e_rxtx.c                   |  88 +-
>   lib/librte_pmd_ixgbe/ixgbe_ethdev.c               | 392 ++++-----
>   lib/librte_pmd_ixgbe/ixgbe_fdir.c                 |  50 +-
>   lib/librte_pmd_ixgbe/ixgbe_pf.c                   | 114 ++-
>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c                 | 276 +++----
>   lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c             |   6 +-
>   lib/librte_pmd_mlx4/mlx4.c                        |   2 +-
>   lib/librte_pmd_null/rte_eth_null.c                |  36 +-
>   lib/librte_pmd_pcap/rte_eth_pcap.c                |   2 +-
>   lib/librte_pmd_ring/rte_eth_ring.c                |  36 +-
>   lib/librte_pmd_virtio/virtio_ethdev.c             | 120 +--
>   lib/librte_pmd_virtio/virtio_rxtx.c               |  20 +-
>   lib/librte_pmd_vmxnet3/vmxnet3_ethdev.c           |  64 +-
>   lib/librte_pmd_vmxnet3/vmxnet3_rxtx.c             |  40 +-
>   lib/librte_pmd_xenvirt/rte_eth_xenvirt.c          |   2 +-
>   51 files changed, 2990 insertions(+), 2483 deletions(-)
>   create mode 100644 lib/librte_eal/common/eal_common_device.c
>   create mode 100644 lib/librte_eal/common/include/rte_common_device.h
>