Re: [dpdk-dev] [EXT] [PATCH v2 1/5] eventdev/rx_adapter: add support to configure event buffer size

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

>Currently Rx event buffer is static array with a default size
>of 192(6*BATCH_SIZE).
>
>``rte_event_eth_rx_adapter_create_with_params`` api is added which
>takes
>``struct rte_event_eth_rx_adapter_params`` to configure event
>buffer size in addition other params . The event buffer is
>allocated from heap after aligning the size to BATCH_SIZE and
>adding 2*BATCH_SIZE. In case of NULL params argument, default event
>buffer size is used.
>
>Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
>Signed-off-by: Ganapati Kundapura <ganapati.kundapura@intel.com>
>
>---
>v2:
>* Updated header file and rx adapter documentation as per review
>comments.
>* new api name is modified as
>rte_event_eth_rx_adapter_create_with_params
>  as per review comments.
>* rxa_params pointer argument Value NULL is allowed to represent the
>  default values
>
>v1:
>* Initial implementation with documentation and unit tests.
>---
> .../prog_guide/event_ethernet_rx_adapter.rst  |  7 ++
> lib/eventdev/rte_event_eth_rx_adapter.c       | 94
>+++++++++++++++++--
> lib/eventdev/rte_event_eth_rx_adapter.h       | 40 +++++++-
> lib/eventdev/version.map                      |  2 +
> 4 files changed, 135 insertions(+), 8 deletions(-)
>
>diff --git a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
>b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
>index 0780b6f711..dd753613bd 100644
>--- a/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
>+++ b/doc/guides/prog_guide/event_ethernet_rx_adapter.rst
>@@ -62,6 +62,13 @@ service function and needs to create an event
>port for it. The callback is
> expected to fill the ``struct rte_event_eth_rx_adapter_conf structure``
> passed to it.
>
>+If the application desires to control the event buffer size, it can use the
>+``rte_event_eth_rx_adapter_create_with_params()`` api. The event
>buffer size is
>+specified using ``struct
>rte_event_eth_rx_adapter_params::event_buf_size``.
>+The function is passed the event device to be associated with the
>adapter
>+and port configuration for the adapter to setup an event port if the
>+adapter needs to use a service function.
>+
> Adding Rx Queues to the Adapter Instance
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c
>b/lib/eventdev/rte_event_eth_rx_adapter.c
>index f2dc69503d..df1653b497 100644
>--- a/lib/eventdev/rte_event_eth_rx_adapter.c
>+++ b/lib/eventdev/rte_event_eth_rx_adapter.c
>@@ -82,7 +82,9 @@ struct rte_eth_event_enqueue_buffer {
> 	/* Count of events in this buffer */
> 	uint16_t count;
> 	/* Array of events in this buffer */
>-	struct rte_event events[ETH_EVENT_BUFFER_SIZE];
>+	struct rte_event *events;
>+	/* size of event buffer */
>+	uint16_t events_size;
> 	/* Event enqueue happens from head */
> 	uint16_t head;
> 	/* New packets from rte_eth_rx_burst is enqued from tail */
>@@ -919,7 +921,7 @@ rxa_buffer_mbufs(struct
>rte_event_eth_rx_adapter *rx_adapter,
> 		dropped = 0;
> 		nb_cb = dev_info->cb_fn(eth_dev_id, rx_queue_id,
> 				       buf->last |
>-				       (RTE_DIM(buf->events) & ~buf-
>>last_mask),
>+				       (buf->events_size & ~buf-
>>last_mask),
> 				       buf->count >= BATCH_SIZE ?
> 						buf->count -
>BATCH_SIZE : 0,
> 				       &buf->events[buf->tail],
>@@ -945,7 +947,7 @@ rxa_pkt_buf_available(struct
>rte_eth_event_enqueue_buffer *buf)
> 	uint32_t nb_req = buf->tail + BATCH_SIZE;
>
> 	if (!buf->last) {
>-		if (nb_req <= RTE_DIM(buf->events))
>+		if (nb_req <= buf->events_size)
> 			return true;
>
> 		if (buf->head >= BATCH_SIZE) {
>@@ -2164,12 +2166,15 @@ rxa_ctrl(uint8_t id, int start)
> 	return 0;
> }
>
>-int
>-rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
>-				rte_event_eth_rx_adapter_conf_cb
>conf_cb,
>-				void *conf_arg)
>+static int
>+rxa_create(uint8_t id, uint8_t dev_id,
>+	   struct rte_event_eth_rx_adapter_params *rxa_params,
>+	   rte_event_eth_rx_adapter_conf_cb conf_cb,
>+	   void *conf_arg)
> {
> 	struct rte_event_eth_rx_adapter *rx_adapter;
>+	struct rte_eth_event_enqueue_buffer *buf;
>+	struct rte_event *events;
> 	int ret;
> 	int socket_id;
> 	uint16_t i;
>@@ -2184,6 +2189,7 @@
>rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
>
> 	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -
>EINVAL);
> 	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
>+
> 	if (conf_cb == NULL)
> 		return -EINVAL;
>
>@@ -2231,11 +2237,30 @@
>rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> 		rte_free(rx_adapter);
> 		return -ENOMEM;
> 	}
>+
> 	rte_spinlock_init(&rx_adapter->rx_lock);
>+
> 	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
> 		rx_adapter->eth_devices[i].dev = &rte_eth_devices[i];
>
>+	/* Rx adapter event buffer allocation */
>+	buf = &rx_adapter->event_enqueue_buffer;
>+	buf->events_size = RTE_ALIGN(rxa_params->event_buf_size,
>BATCH_SIZE);
>+
>+	events = rte_zmalloc_socket(rx_adapter->mem_name,
>+				    buf->events_size * sizeof(*events),
>+				    0, socket_id);
>+	if (events == NULL) {
>+		RTE_EDEV_LOG_ERR("Failed to allocate mem for event
>buffer\n");
>+		rte_free(rx_adapter->eth_devices);
>+		rte_free(rx_adapter);
>+		return -ENOMEM;
>+	}
>+
>+	rx_adapter->event_enqueue_buffer.events = events;
>+
> 	event_eth_rx_adapter[id] = rx_adapter;
>+
> 	if (conf_cb == rxa_default_conf_cb)
> 		rx_adapter->default_cb_arg = 1;
> 	rte_eventdev_trace_eth_rx_adapter_create(id, dev_id,
>conf_cb,
>@@ -2243,6 +2268,57 @@
>rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> 	return 0;
> }
>
>+int
>+rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
>+				rte_event_eth_rx_adapter_conf_cb
>conf_cb,
>+				void *conf_arg)
>+{
>+	struct rte_event_eth_rx_adapter_params rxa_params;
>+
>+	/* Event buffer with default size = 6*BATCH_SIZE */

Why is it a multiple of 6, if its not documented please add a line here.

>+	rxa_params.event_buf_size = ETH_EVENT_BUFFER_SIZE;
>+	return rxa_create(id, dev_id, &rxa_params, conf_cb, conf_arg);
>+}
>+
>+int
>+rte_event_eth_rx_adapter_create_with_params(uint8_t id, uint8_t
>dev_id,
>+			struct rte_event_port_conf *port_config,
>+			struct rte_event_eth_rx_adapter_params
>*rxa_params)
>+{
>+	struct rte_event_port_conf *pc;
>+	int ret;
>+	struct rte_event_eth_rx_adapter_params temp_params = {0};
>+
>+	if (port_config == NULL)
>+		return -EINVAL;
>+
>+	/* use default values if rxa_parmas is NULL */
>+	if (rxa_params == NULL) {
>+		rxa_params = &temp_params;
>+		rxa_params->event_buf_size =
>ETH_EVENT_BUFFER_SIZE;
>+	}
>+
>+	if (rxa_params->event_buf_size == 0)
>+		return -EINVAL;
>+
>+	pc = rte_malloc(NULL, sizeof(*pc), 0);
>+	if (pc == NULL)
>+		return -ENOMEM;
>+
>+	*pc = *port_config;
>+
>+	/* event buff size aligned to BATCH_SIZE + 2*BATCH_SIZE */
>+	rxa_params->event_buf_size = RTE_ALIGN(rxa_params-
>>event_buf_size,
>+					       BATCH_SIZE);
>+	rxa_params->event_buf_size += BATCH_SIZE + BATCH_SIZE;
>+
>+	ret = rxa_create(id, dev_id, rxa_params, rxa_default_conf_cb,
>pc);
>+	if (ret)
>+		rte_free(pc);
>+
>+	return ret;
>+}
>+
> int
> rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
> 		struct rte_event_port_conf *port_config)
>@@ -2252,12 +2328,14 @@ rte_event_eth_rx_adapter_create(uint8_t
>id, uint8_t dev_id,
>
> 	if (port_config == NULL)
> 		return -EINVAL;
>+
> 	RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -
>EINVAL);
>
> 	pc = rte_malloc(NULL, sizeof(*pc), 0);
> 	if (pc == NULL)
> 		return -ENOMEM;
> 	*pc = *port_config;
>+
> 	ret = rte_event_eth_rx_adapter_create_ext(id, dev_id,
> 					rxa_default_conf_cb,
> 					pc);
>@@ -2286,6 +2364,7 @@ rte_event_eth_rx_adapter_free(uint8_t id)
> 	if (rx_adapter->default_cb_arg)
> 		rte_free(rx_adapter->conf_arg);
> 	rte_free(rx_adapter->eth_devices);
>+	rte_free(rx_adapter->event_enqueue_buffer.events);
> 	rte_free(rx_adapter);
> 	event_eth_rx_adapter[id] = NULL;
>
>@@ -2658,6 +2737,7 @@ rte_event_eth_rx_adapter_stats_get(uint8_t
>id,
>
> 	stats->rx_packets += dev_stats_sum.rx_packets;
> 	stats->rx_enq_count += dev_stats_sum.rx_enq_count;
>+
> 	return 0;
> }
>
>diff --git a/lib/eventdev/rte_event_eth_rx_adapter.h
>b/lib/eventdev/rte_event_eth_rx_adapter.h
>index 3f8b362295..a7881097b4 100644
>--- a/lib/eventdev/rte_event_eth_rx_adapter.h
>+++ b/lib/eventdev/rte_event_eth_rx_adapter.h
>@@ -26,6 +26,7 @@
>  * The ethernet Rx event adapter's functions are:
>  *  - rte_event_eth_rx_adapter_create_ext()
>  *  - rte_event_eth_rx_adapter_create()
>+ *  - rte_event_eth_rx_adapter_create_with_params()
>  *  - rte_event_eth_rx_adapter_free()
>  *  - rte_event_eth_rx_adapter_queue_add()
>  *  - rte_event_eth_rx_adapter_queue_del()
>@@ -36,7 +37,7 @@
>  *
>  * The application creates an ethernet to event adapter using
>  * rte_event_eth_rx_adapter_create_ext() or
>rte_event_eth_rx_adapter_create()
>- * functions.
>+ * or rte_event_eth_rx_adapter_create_with_params() functions.
>  * The adapter needs to know which ethernet rx queues to poll for
>mbufs as well
>  * as event device parameters such as the event queue identifier,
>event
>  * priority and scheduling type that the adapter should use when
>constructing
>@@ -256,6 +257,16 @@ struct
>rte_event_eth_rx_adapter_vector_limits {
> 	 */
> };
>
>+/**
>+ * A structure to hold adapter config params
>+ */
>+struct rte_event_eth_rx_adapter_params {
>+	uint16_t event_buf_size;
>+	/**< size of event buffer for the adapter.
>+	 * the size is aligned to BATCH_SIZE and added (2 * BATCH_SIZE)
>+	 */

BATCH_SIZE is internal i.e. not exposed to application layer, can we please define
what is BATCH_SIZE here and why applications input needs to be aligned to BATCH_SIZE?
and why 2*BATCH size is required.

If this is an application driven parameter we shouldn't impose arbitrary constraints like this.
Maybe "Aligned to power of 2" would be sufficient.

OR

Extend rte_event_eth_rx_adapter_caps_get() to return optimal event_buf_size if supported.


>+};
>+
> /**
>  *
>  * Callback function invoked by the SW adapter before it continues
>@@ -356,6 +367,33 @@ int
>rte_event_eth_rx_adapter_create_ext(uint8_t id, uint8_t dev_id,
> int rte_event_eth_rx_adapter_create(uint8_t id, uint8_t dev_id,
> 				struct rte_event_port_conf
>*port_config);
>
>+/**
>+ * This is a variant of rte_event_eth_rx_adapter_create() with
>additional
>+ * adapter params specified in ``struct
>rte_event_eth_rx_adapter_params``.
>+ *
>+ * @param id
>+ *  The identifier of the ethernet Rx event adapter.
>+ *
>+ * @param dev_id
>+ *  The identifier of the event device to configure.
>+ *
>+ * @param port_config
>+ *  Argument of type *rte_event_port_conf* that is passed to the
>conf_cb
>+ *  function.
>+ *
>+ * @param rxa_params
>+ *  Pointer to struct rte_event_eth_rx_adapter_params.
>+ *  In case of NULL, default values are used.
>+ *
>+ * @return
>+ *   - 0: Success
>+ *   - <0: Error code on failure
>+ */
>+__rte_experimental
>+int rte_event_eth_rx_adapter_create_with_params(uint8_t id,
>uint8_t dev_id,
>+			struct rte_event_port_conf *port_config,
>+			struct rte_event_eth_rx_adapter_params
>*rxa_params);
>+
> /**
>  * Free an event adapter
>  *
>diff --git a/lib/eventdev/version.map b/lib/eventdev/version.map
>index cd86d2d908..87586de879 100644
>--- a/lib/eventdev/version.map
>+++ b/lib/eventdev/version.map
>@@ -138,6 +138,8 @@ EXPERIMENTAL {
> 	__rte_eventdev_trace_port_setup;
> 	# added in 20.11
> 	rte_event_pmd_pci_probe_named;
>+	# added in 21.11
>+	rte_event_eth_rx_adapter_create_with_params;
>
> 	#added in 21.05
> 	rte_event_vector_pool_create;
>--
>2.25.1