Hi Pavan,

-----Original Message-----
From: Pavan Nikhilesh Bhagavatula <redacted>
Sent: Wednesday, September 22, 2021 1:55 AM
To: Naga Harish K, S V <redacted>; Jerin Jacob
Kollanukkaran [off-list ref]; Jayatheerthan, Jay
[off-list ref]
Cc: dev@dpdk.org; Kundapura, Ganapati <redacted>
Subject: RE: [EXT] [dpdk-dev] [PATCH v2 1/5] eventdev/rx_adapter: add
support to configure event buffer size

quoted

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 <redacted>
Signed-off-by: Ganapati Kundapura <redacted>

---
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

quoted

+921,7 @@ rxa_buffer_mbufs(struct rte_event_eth_rx_adapter

*rx_adapter,

quoted

		dropped = 0;
		nb_cb = dev_info->cb_fn(eth_dev_id, rx_queue_id,
				       buf->last |
-				       (RTE_DIM(buf->events) & ~buf-

quoted

last_mask),

+				       (buf->events_size & ~buf-

quoted

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);

quoted

	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,

@@

quoted

-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.

This is the existing default event buffer size and continued its usage.
There is no hard rule to decide the default size.

quoted

+	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-

quoted

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 @@

quoted

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);

quoted

	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.

BATCH_SIZE is the size used to dequeue packets form NIC rx queues. 
If the event buffer size aligned to BATCH_SIZE, the buffer utilization will be upto
Its fullest and avoid wastage of buffer space during rollover conditions.
The additional adjustment is to make sure that, the buffer usage is at least upto
The user requested size under overload conditions.
 

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.

quoted

+};
+
/**
 *
 * 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