Re: [RFC PATCH 0/4] pktdev

[Marc Sune <marc.sune-kpkqNMk1I7M@public.gmane.org>]

On 17/04/15 17:16, Bruce Richardson wrote:
> Hi all,
>
> to continue this discussion a bit more, here is my, slightly different, slant
> on what a pktdev abstraction may look like.
>
> The primary objective I had in mind when drafting this is to provide the
> minimal abstraction that can be *easily* used as a common device abstraction for
> existing (and future) device types to be passed to dataplane code. The patchset
> demonstrates this by defining a minimal interface for pktdev - since I firmly
> believe the interface should be as small as possible - and then showing how that
> common interface can be used to unify rings and ethdevs under a common API for the
> datapath. I believe any attempt to unify things much beyond this to the control
> plane or setup phase is not worth doing - at least not initially - as at
> init time the code always needs to be aware of the underlying resource type in
> order to configure it properly for dataplane use.
>
> The overall objective I look to achieve is illustrated by the final patch in
> the series, which is a sample app where the same code is used for all cores,
> irrespective of the underlying device type.
>
> To get to that point, patch 1 defines the minimal API - just RX and TX. The .c
> file in the library is empty for simplicity, though I would see some
> functionality moving there when/if it makes sense e.g. the callback support
> from ethdev, as is done in Keith's patchset.
> Patch 2 then makes very minimal changes to ethdev to allow ethdevs to be used
> as pktdevs, and to make use of the pktdev functions when appropriate
> Patch 3 was, for me, the key test for this implementation - how hard was it to
> make an rte_ring usable as a pktdev too. Two single-line functions for RX/TX
> and a separate "converter" function proved to be all that was necessary here -
> and I believe simpler solutions may be possible too, as the extra structures
> allocated on conversion could be merged into the rte_ring structure itself and
> initialized on ring creation if we prefer that option. It is hoped/presumed that
> wrapping other structures, such as KNI, may prove to be just as easily done.
> [Not attempted yet - left as an exercise for the reader :-)].
>
> Now, in terms of pktdev vs ethdev, there is nothing in this proposal that
> cannot also be done using ethdev AFAIK. However, pktdev as outlined here
> should make the process far easier than trying to create a full PMD for something.
> All NIC specific functions, including things like stop/start, are stripped out,
> as they don't make sense for an rte_ring or other software objects.
> Also, the other thing this provides is that we can move away from just using
> port ids. Instead in the same way as we now reference rings/mempools/KNIs etc
> via pointer, we can do the same with ethernet ports as pktdevs on the data path.
> There was discussion previously on moving beyond 8-bit port ids. If we look to
> use ethdev as a common abstraction, I feel that change will soon have to be made
> causing a large amount of code churn.

Hi Richard,

First thank you both for taking the time to look at this. I did not not 
reply to Keith because you Richard summarized most of my concerns.

I had a brief look to this second proposal. It is more aligned to what I 
had in mind. But still I feel it is slightly too complicated. I don't 
like much the necessary (in your approach) MACRO-like pkt_dev_data 
struct. It is also slightly inconvenient that the user has to do:

+	struct rte_pkt_dev *in = rte_eth_get_dev(0);

+		struct rte_pkt_dev *out = rte_ring_get_dev(
+				rte_ring_create(name, 4096, rte_socket_id(), 0));



What about something like (~pseudo-code):

rte_pkt_dev_data.h:

    enum rte_pkt_dev_type{
         RTE_PKT_DEV_ETH,
         RTE_PKT_DEV_RING,
         RTE_PKT_DEV_KNI,
         //Keep adding as more PMDs are supported
    };


    //This struct may be redundant if there is nothing more
    struct rte_pkt_dev_data{
         enum rte_pkt_dev_type;
         //Placeholder, maybe we need more...
    };

    //Make RX/TX pktdev APIs more readable, but not really needed
    typedef void pkt_dev_t;

(In all PMDs and e.g. KNI and RINGs):

  struct rte_eth_dev {
     struct rte_pkt_dev_data pkt_dev;// <++++++++++++++++++++++++++++++++++
     eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function. */
     eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function. */
     struct rte_eth_dev_data *data;  /**< Pointer to device data */
     /...

rte_pkt_dev.h:

       #include <rte_ethdev.h>
       //Include PMD (and non-PMD) TX/RX headers...

    static inline uint16_t
    rte_pkt_tx_burst(pkt_dev_t* dev, uint16_t queue_id,
              struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
    {
         switch (((struct rte_pkt_dev_data*)dev)->type){
             case  RTE_PKT_DEV_ETH:
                 struct rte_eth_dev* eth_dev = (struct rte_eth_dev*)pkt_dev;
                 rte_pkt_tx_burst(eth_dev, queue_id, tx_pkts, nb_pkts);
                 break;
             case RTE_PKT_DEV_RING:
                 //...
         }
    }

    //...


Maybe it is oversimplified? With this approach tough, we would barely 
touch the PMDs and the functionality can be built on top of the existing 
PMDs.

Thoughts?
Marc


>
> Bruce Richardson (4):
>    Add example pktdev implementation
>    Make ethdev explicitly a subclass of pktdev
>    add support for a ring to be a pktdev
>    example app showing pktdevs used in a chain
>
>   config/common_bsdapp           |   5 +
>   config/common_linuxapp         |   5 +
>   examples/pktdev/Makefile       |  57 +++++++++++
>   examples/pktdev/basicfwd.c     | 222 +++++++++++++++++++++++++++++++++++++++++
>   lib/Makefile                   |   1 +
>   lib/librte_ether/rte_ethdev.h  |  26 ++---
>   lib/librte_pktdev/Makefile     |  56 +++++++++++
>   lib/librte_pktdev/rte_pktdev.c |  35 +++++++
>   lib/librte_pktdev/rte_pktdev.h | 144 ++++++++++++++++++++++++++
>   lib/librte_ring/rte_ring.c     |  41 ++++++++
>   lib/librte_ring/rte_ring.h     |   3 +
>   11 files changed, 582 insertions(+), 13 deletions(-)
>   create mode 100644 examples/pktdev/Makefile
>   create mode 100644 examples/pktdev/basicfwd.c
>   create mode 100644 lib/librte_pktdev/Makefile
>   create mode 100644 lib/librte_pktdev/rte_pktdev.c
>   create mode 100644 lib/librte_pktdev/rte_pktdev.h
>