Re: [RFC PATCH 0/4] pktdev

[Bruce Richardson <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Mon, Apr 20, 2015 at 08:51:26AM +0200, Marc Sune wrote:
> 
> 
> On 17/04/15 21:50, Wiles, Keith wrote:
> >Hi Marc and Bruce,
> 
> Hi Keith, Bruce,
> 
> >
> >On 4/17/15, 1:49 PM, "Marc Sune" <marc.sune-kpkqNMk1I7M@public.gmane.org> wrote:
> What I was proposing is to try to add the minimum common shared state in
> order to properly demultiplex the RX/TX call and have a common set of
> abstract calls (the pkt_dev type). In a way, I was proposing to deliberately
> not have a shared struct rte_dev_data because I think the internals of the
> "pkt_dev" can be very different across devices (e.g. queues in kni vs eth
> port vs. crypto?). I treat the pkt_dev as a "black box" that conforms to
> TX/RX API, leaving the developer of that device to define its internal
> structures as it better suites the needs. I only use each of the specific
> device type TX/RX APIs (external to us, pkt_dev library) in rte_pkt_dev.h.
> This also simplifies the refactor required to eventually integrate the
> rte_pkt_dev library and builds it "on top" of the existing APIs.
> 
> The other important difference with both, Bruce and your approach, and mine
> is the use of function pointers for RX/TX. I don't use them, which makes the
> entire abstracted TX/RX (including the final RX/TX routines itself)
> functions be "inlinable".
> 
> Btw, I forgot to add something basic in the previous pseudo-code. The
> different types have to be conditionally compiled according to compiled-in
> DPDK libs:
> 
> rte_pkt_dev.h:
> 
>     #include <rte_config.h>
> 
>     //Eth devices
>     #ifdef RTE_LIBRTE_ETHER
>     #include <rte_ethdev.h>
>     #endif
> 
>     //KNI
>     #ifdef RTE_LIBRTE_KNI
>     #include <rte_kni.h>
>     #endif
> 
>     //...
>     //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){
>             #ifdef RTE_LIBRTE_ETHER
>             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;
>             #endif
> 
>             #ifdef RTE_LIBRTE_KNI
>             case RTE_PKT_DEV_KNI:
>                 //...
>                 break;
>             #endif
> 
>             default:
>                     //Corrupted type or unsupported (without compiled
> support)
>                     //Ignore or fail(fatal error)?
>                     break;
>         }
>    }
> 
>    //...

Yes, this is an interesting approach, and with the inlining could indeed be
less overhead for the ring and kni compared to my suggestion due to the inlining.
There might be a slight overhead for the RX/TX ethdev functions though - 1/2
cycles due to the extra (hopefully predictable) branch in the RX/TX call, since
we always need the indirect function call for the PMDs.

I also like the use of pointers rather than port ids.

Let me think on this a bit more.

/Bruce