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

[Jerin Jacob]

> > > 
> > > 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
> > 
> 
> I have had the application specify the name. so that it can call
> 
> struct rte_service_spec *rte_service_get_by_name(const char *name);

OK. I was thinking like there will be fixed name for the rx_adapter
service like "eth_rx_adapter_%d"(%d for adapter id) which be exposed in
rte_event_eth_rx_adapter.h.
It can help in removing cfg.service_name. I am fine with either way.

> 
> followed by
> int32_t rte_service_enable_on_lcore(struct rte_service_spec *service,
>                                    uint32_t lcore);
> 
> given that the application knows the socket id of the event device
> associated with the adapter.
> 
> > > 
> > > // 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
> > 
> 
> Doesn't the capability of a <eventdev, ethdev> also need to be made
> available to the application as an adapter API ?

Yes. Make sense to expose as adapter API also.

> 
> for e.g., so the application can call rte_event_eth_rx_adapter_queue_add()
> with rx_queue_id = -1 if RX_ADAPTER_CAP_ADD_QUEUE is not set.
> 
> In the same manner, if I understand RX_ADAPTER_CAP_SET_FLOW_ID, the
> application would have provide a flow ID in
> rte_event_eth_rx_adapter_queue_add(), if RX_ADAPTER_CAP_SET_FLOW_ID is not
> set.
> 
> Also if a given <eventdev, ethdev> are connected in HW then is the servicing
> weight of a queue applicable ?

It is not applicable. If it connected in HW, We can ignore it.

> 
> > 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?
> 
> Yes.
> 
> > 
> > 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.
> 
> Yes.
> > 
> > 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.
> 
> Wouldnt the rx_packets be the sum of the service core thread packet count
> and the count provided by the eventdev pmd ops and be obtained from a call
> to

Yes.

> 
> rte_event_eth_rx_adapter_stats_get(uint8_t id,
> 	struct rte_event_eth_rx_adapter_stats *stats);
> 
> > 
> > 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.
> > 
> 
> One solution is:
> 
> struct rte_event_eth_rx_adapter_conf {
>     uint8_t dev_id;
> 
>     int (*conf_cb)(uint8_t id, uint8_t port_id, uint32_t flags, struct
> rte_event_eth_rx_adapter_conf *conf);
> 
>     unsigned int max_nb_rx;
> 
>     int event_port_id;
> 
>     char service_name[];
> }
> 
> Where dev_id and conf_cb have to be specified in the create call, but
> event_port_id and service_name will be filled in when conf_cb() is invoked

I was thinking like event_port_id will be rte_event_port_count() + 1.
ie When adapter needs the additional port, It can
- stop the eventdev
- reconfigure with rte_event_queue_count() , rte_event_port_count() + 1
- start the eventdev.

The only problem with callback is that all the application needs to
implement it. If you think, application need more control then we can
expose callback and if it is NULL then default handler can be called in
common code.


> if required. flags will specify what parameters of conf need to be filled
> in. The conf parameter can be separated into a different
> 
> struct rte_event_eth_rx_adapter_ext_conf {
>     unsigned int max_nb_rx;
> 
>     int event_port_id;
> 
>     char service_name[];
> }
> 
> Nikhil
>