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

-----Original Message-----

Date: Mon, 10 Jul 2017 11:44:10 +0530
From: "Rao, Nikhil" <redacted>
To: Jerin Jacob <redacted>
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 [off-list ref]
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

Hi Jerin,

Hi Nikhil,

thanks for your feedback, further comments below.

On 7/7/2017 9:27 PM, Jerin Jacob wrote:

quoted

-----Original Message-----

quoted

Date: Fri, 7 Jul 2017 20:33:19 +0530
From: Jerin Jacob <redacted>

quoted

quoted

/* 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)

I just provided a name to share the view. You can choose better name.

OK.

quoted

quoted

quoted

quoted

int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t

eth_port_id);

I also think that the application should be able to call create() with > 1
ports. This would allow a single service to poll multiple NICs with WRR
priority.

Good point.

Can we realize the same use case like below?
- Instead of applying WRR over multiple NIC ports and adding the logic in Rx
  adapter, How about applying the WRR over multiple service function and
  move the WRR logic to service function layer.

i.e
one adapter is
- one service function(adapter_queue_add() will be used to add more
  queues)
- one constant set of ops.

Advantages:
- WRR over service functions will be useful as other service functions
can utilize it as it is not strictly specific to Rx adapter.
- In order to work with, below mentioned use cases, RX adapter ops needs
  to be constant and it will decided on the _adapter_create where "eth_port_id"
and "dev_id" specified.

1) Ethdev HW is not capable of injecting the packets and SW eventdev
driver(All existing ethdev PMD + drivers/event/sw PMD combination)
2) Ethdev HW is not capable of injecting the packets and not compatible
HW eventdev driver(All existing ethdev PMD + driver/event/octeontx PMD
combination)
3) Ethdev HW is capable of injecting the packet to compatible
HW eventdev driver.

- it will remove the below side effect(queue add/del API needs port_id)

Thoughts?

The side effect of is that the queue add/del APIs would need to specify the
port_id as well.

quoted

quoted

quoted

quoted

int rte_event_eth_rx_adapter_get_info(uint8_t id, struct

rte_event_eth_rx_adap_info *info);

quoted

int rte_event_eth_rx_adapter_configure(uint8_t id, struct

rte_event_eth_rx_adap_config *cfg);

quoted

int rte_event_eth_rx_adapter_queue_add(uint8_t id, int32_t rx_queue_id,

const struct rte_eth_rx_event_adapter_queue_config *config);

quoted

int rte_event_eth_rx_adapter_queue_del(uint8_t id, int32_t rx_queue_id)

quoted

quoted

quoted

quoted

int rte_event_eth_rx_adapter_run();

The adapter's run function is not part of the public interface, agree ?

Agree.

quoted

quoted

quoted

quoted

int rte_event_eth_rx_adapter_free(uint8_t id);

If I understood your idea, the function pointer struct would look something
like this.

struct rte_eventdev_rx_adapter_ops {
	rx_adapter_create_t 	create,
	rx_adapter_get_info_t	info,
	rx_adapter_configure_t	configure,
	rx_adapter_queue_add_t  queue_add,
	rx_adapter_queue_del_t	queue_del,
	rx_adapter_queue_free_t	queue_free,
       rx_adapter_free_t	free
};

Yes. But, I think, adapter create and free goes in struct rte_eventdev_op .
See below.

quoted

struct rte_eventdev {
        ..
        const struct rte_eventdev_rx_adapter_ops *rx_adapter_ops;
        ..
};

An eventdev instance can have N adapters not just one. So this will be
pointer to pointer, indexed through adapter_id.

In SW PMD case, the eventdev may have only one adapter.
In HW PMD cases, There will more than one. example,
- if octeontx eventdev dev_id + any external PCI n/w card like i40e or
nicvf case an adapter with similar adapter ops as SW PMD
- if octeontx eventdev dev_id + octeontx ethdev dev_id case another
adapter that does not need service cores to inject event to octeontx
eventdev

- Your generic Rx adapter we will make it as common code so that both SW PMD and
octeontx PMD in service core mode as use the functions and register the
ops.

OK.

quoted

quoted

I was thinking like this

int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id, uint8_t eth_port_id)
{
	eventdev_ops = ... /* access through dev_id */

	/* common code to get ops memory from adapter id */
	struct rte_eventdev_rx_adapter_ops* ops = rte_event_pmd_adapter_allocate(id);

	/* Fill in adapter ops from driver */
	eventdev_ops->create_adapter(ops, dev_id, eth_port_id)

As an implementation detail, the function pointer setup needs to account for
DPDK primary/secondary processes, i.e, function addresses could be different
in the 2 processes.

Yes

quoted

quoted

}

int rte_event_eth_rx_adapter_get_info(uint8_t id, struct rte_event_eth_rx_adap_info *info)
{

Argument list is missing the dev_id (as are the
configure/queue_add/queue_del) functions.

Yes

quoted

quoted

	eventdev_ops = ... /* access through dev_id */

	struct rte_eventdev_rx_adapter_ops* ops = eventdev_ops->rx_adapter_ops[id];

	/* adapter specific info ops)
	ops->info(ops,....);

}

IMO, the mapping with your current proposal may go like this.

- rte_event_eth_rx_adapter_create() // Will just fill the adapter ops from driver, probably
   your existing rte_event_eth_rx_adapter_init() can go here
- exiting rte_event_eth_rx_adapter_create() will be more of
proposed rte_event_eth_rx_adapter_configure() on the given adapter.
- existing rte_event_eth_rx_adapter_queue_add() maps 1:1. But you can
   remove eth_dev_id as it is provided proposed rte_event_eth_rx_adapter_create()

See above for the case where the application could call create() with 2
different ports.

quoted

- same applies to rte_event_eth_rx_adapter_queue_del()
- exiting rte_event_eth_rx_adapter_stats_get() can be changed to
   "xstat"(more like eventdev xstat scheme) so based on the adapter
capability application can get stats)


flow shall be:
1) application calls rte_event_eth_rx_adapter_create(id, eventdev_id,
ethdev_id) to create the adapter.(Drivers fills the adapter ops based on
eventdev_id + ethdev_id capabilities)
2) application calls rte_event_eth_rx_adapter_info_get() to get the
capability and other information of the adapter.
3) application calls rte_event_eth_rx_adapter_configure() to configure
based on the application need and the adapter capability.
4) application calls rte_event_eth_rx_adapter_queue_add() to link ethdev queues to
eventdev queues. On this call, The driver creates the service functions or programs the HW registers
based on the adapter capability to take the events from ethdev and inject to eventdev.

The flow looks good to me.

Great!!

quoted

quoted

quoted

But from previous emails (see struct rte_event_dev_producer_conf below), it
appears that the rx queue id and event information would be needed to create
the adapter that enqueues from the ethdev queue id to the event pmd, however
that information is available only at queue add time - thoughts ?

Just eventdev_id and ethdev_id is enough to create adapter.

OK.