Re: [RFC PATCH 0/4] pktdev

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

Bruce,

On 20/04/15 12:43, Bruce Richardson wrote:
> 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 guess if the user application uses multiple types of port (pkt devs), 
which is basically when you get a benefit out of the abstracted API, 
they likely are doing a similar if / switch statement.

Ofc, with this approach single type pkt-dev applicationss can always use 
the "lower-level" APIs remain the same, as of now.

>
> I also like the use of pointers rather than port ids.
>
> Let me think on this a bit more.

If you (both, Keith&you) think it is worth, I can propose an RFC patch 
based on (v3?).

Marc

>
> /Bruce