Re: [dpdk-dev] [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Thu, 27 Jul 2017 16:28:29 +0530
> From: "Rao, Nikhil" <nikhil.rao@intel.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: gage.eads@intel.com, dev@dpdk.org, thomas@monjalon.net,
>  bruce.richardson@intel.com, harry.van.haaren@intel.com,
>  hemant.agrawal@nxp.com, nipun.gupta@nxp.com, narender.vangati@intel.com,
>  Abhinandan Gujjar <abhinandan.gujjar@intel.com>, nikhil.rao@intel.com
> Subject: Re: [PATCH 1/2] eventdev: add event adapter for ethernet Rx queues
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.2.1
> 
> 
> 
> In the case of a SW thread we would like to use the servicing weight
> specified in the queue to do WRR across <ports, queues[]>, in keeping with

OK, then lets work together to address in transparent manner where it
works for HW and SW.

> the adaper per <eventdev, eth port> model, one way to do this is to use the
> same cfg.service_name in the rte_event_eth_rx_adapter_configure() call.
> 
> However this creates a few difficulties/inconsistencies:

I agree. If we are thinking about WRR across <ports,queues[]> then above
proposal implementation creates inconsistencies. On the other side, it create challenges
with HW implementation to have unified adapter API works for both HW and SW.

> 
> 1)Service has the notion of a socket id. Multiple event dev IDs can be
> included in the same service, each event dev has a socket ID -> this seems
> to be an inconsistency that shouldn’t be allowed by design.
> 
> 2)Say, the Rx event adapter doesn’t drop packets (could be configurable),
> i.e,  if events cannot be enqueued into the event device, these remain in a
> buffer, when the buffer fills up packets aren’t dequeued from the eth
> device.
> 
> In the simplest case the Rx event adapter service has a single <event
> device, event port> across multiple eth ports, it dequeues from the wrr[]
> and buffers events, bulk enqueues BATCH_SIZE events into the <event device,
> event port>.
> 
> With adapters having different <event device, event port> code can be
> optimized so that adapters that have a common <event device, event port> can
> be made to refer to a common enqueue buffer { event dev, event port, buffer
> } structure but this adds more book keeping in the code.
> 
> 3)Every adapter can be configured with max_nb_rx ( a max nb of packets that
> it can process in any invocation) – but the max_nb_rx seems like a service
> level parameter instead of it being a summation across adapters.
> 
> 1 & 3 could be solved by restricting the adapters to the same (as in the
> first rte_event_eth_rx_adapter_configure() call) socket ID, and perhaps
> using the max value of max_nb_rx or using the same value of max_nb_rx across
> adapters. #2 is doable but has a bit of code complexity to handle the
> generic case.
> 
> Before we go there, I wanted to check if there is an alternative possible
> that would remove the difficulties above. Essentially allow multiple ports
> within an adapter but avoid the problem of the inconsistent <eventdev, port>
> combinations when using multiple ports with a single eventdev.
> 
> Instead of
> ==
> rte_event_eth_rx_adapter_create()
> rte_event_eth_rx_adapter_get_info();
> rte_event_eth_rx_adapter_configure();
> rte_event_eth_rx_adapter_queue_add();
> ==
> 
> How about ?
> ==
> 
> rte_event_eth_rx_adapter_get_info(uint8_t dev_id, uint8_t eth_port_id,
>         struct rte_event_eth_rx_adap_info *info);
> 
> struct rte_event_eth_rx_adap_info {
>         uint32_t cap;
> 
> /* adapter has inbuilt port, no need to create producer port */
> #define RTE_EVENT_ETHDEV_CAP_INBUILT_PORT  (1ULL << 0)
> /* adapter does not need service function */
> #define RTE_EVENT_ETHDEV_CAP_NO_SERVICE_FUNC (1ULL << 1)
> 
> }
> 
> rte_event_eth_rx_adapter_conf cfg;
> cfg.event_port = event_port;
> cfg.service_name = “rx_adapter_service”;

Does application need to specify the service name? IMO, it better a
component(rx_adapter) defines it name and format and expose in rte_event_eth_rx_adapter.h

> 
> // all ports in eth_port_id[] have cap =
> //!RTE_EVENT_ETHDEV_CAP_INBUILT_PORT
> // && ! RTE_EVENT_ETHDEV_CAP_NO_SERVICE_FUNC
> rte_event_eth_rx_adapter_create(dev_id, eth_port_id[], N, id, &cfg);

The downside might be:
- application has different flow based on based on the capability.
Listing down a few capabilities/limitation below.

> ===
> int rte_event_eth_rx_adapter_queue_add() would need a port id in the N>1
> port case, that can be ignored if the adapter doesn’t need it (N=1).
> 
> thanks for reading the long email, thoughts ?

I have bit another thought to solve the above mentioned downside.

- Theme is based on your original rx adapter proposal but with eventpmd
  ops(not adapter ops).i.e Reuse as much of your existing Rx adapter
implementation as common code and add hooks for HW based adapters. For
example, before you add  <ethdev, queue_id> to "rx_poll" in eth_poll_wrr_calc(),
Check for eventdev PMD ops is available adding it HW. If yes, Don't add in "rx_poll"

adapter_api
------------
int rte_event_eth_rx_adapter_create(id, rte_event_eth_rx_adapter_conf *conf)
int rte_event_eth_rx_adapter_queue_add(uint8_t id, uint8_t eth_dev_id, int32_t rx_queue_id, rte_event_eth_rx_adapter_queue_conf *conf);

eventdev PMD op api(not as adapter PMD as discussed earlier)
-------------------

1) typedef uint64_t (*eventdev_rx_adap_capa)(struct rte_eventdev *dev,  uint8_t ethdev_id)

Return the adapter capability of a given eventdev when it needs to
connected to a specific ethdev_id

Possible capability values based on my understating for existing SW and Cavium
HW PMD. NXP folks can add new ones.

- RX_ADAPTER_CAP_INBUILT_PORT - /* adapter has inbuilt port, no need to create producer port by common code */
- RX_ADAPTER_CAP_SET_FLOW_ID  - /* adapter capable of setting RTE_ETH_RX_EVENT_ADAPTER_QUEUE_FLOW_ID_VALID */
- RX_ADAPTER_CAP_ADD_QUEUE /* adapter capable of adding any specific
ethdev rx queue to any eventdev queue. Some eventdev PMD has a limitation
that once a < ethdev_id , queue_id> connected to specific eventdev queue,
all the all queues_id under the same ethdev_id need to be connected to
same eventdev queue. aka works only on the rte_event_eth_rx_adapter_queue_conf.rx_queue_id == -1 mode, */


2) typedef int (*eventdev_rx_adap_add)(struct rte_eventdev *dev,  uint8_t ethdev_id, int queue_id, rte_event_eth_rx_adapter_queue_conf *conf));
-- if implemented by eventdev PMD and returns zero then COMMON code does not need to poll */


3) typedef int (*eventdev_rx_adap_del)(struct rte_eventdev *dev,  uint8_t ethdev_id, int queue_id)
-- remove previously added


*** Haven't spend a lot of time on API/macro name.Please use better naming conversion.


Another notes based on your existing implementation  + eventdev ops scheme

1) rte_event_eth_rx_adapter_creates() registers service function by
default. It should be delayed to when common adapter code find a device
with !RX_ADAPTER_CAP_INBUILT_PORT cap on rte_event_eth_rx_adapter_queue_add()

2) Do we need rx_adapter start/stop functions?

3) If it happens to be case where rte_event_eth_rx_adapter_queue_add()
use only RX_ADAPTER_CAP_INBUILT_PORT then common code should not
create any service.

4) If adapter uses one port with service core and other one with HW
adapter. rte_event_eth_rx_adapter_stats.rx_packets will be not updated
correctly, We need eventdev PMD ops to get those stats. If we agree
overall PMD ops + adapter API partitioning then we can refine additionally
eventpmd for stats etc or xstat based scheme etc.

5) specifying rte_event_eth_rx_adapter_conf.rx_event_port_id on
rte_event_eth_rx_adapter_create() would waste one HW eventdev port if its
happen to be used RX_ADAPTER_CAP_INBUILT_PORT on rte_event_eth_rx_adapter_queue_add().
unlike SW eventdev port, HW eventdev ports are costly so I think, We
need to have another eventdev PMD ops to create service/producer ports.
Or any other scheme that creates rte_event_eth_rx_adapter_conf.rx_event_port_id
on demand by common code.

Thoughts?