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

["Naga Harish K, S V" <s.v.naga.harish.k@intel.com>]

Hi Pavan,

> -----Original Message-----
> From: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
> Sent: Wednesday, September 22, 2021 1:55 AM
> To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Jayatheerthan, Jay
> <jay.jayatheerthan@intel.com>
> Cc: dev@dpdk.org; Kundapura, Ganapati <ganapati.kundapura@intel.com>
> Subject: RE: [EXT] [dpdk-dev] [PATCH v2 1/5] eventdev/rx_adapter: add
> support to configure event buffer size
> 
> >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.

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

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

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