Re: [dpdk-dev] [PATCH v5 1/5] eventdev/rx_adapter: add event buffer size configurability

["Jayatheerthan, Jay" <jay.jayatheerthan@intel.com>]

> -----Original Message-----
> From: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> Sent: Monday, October 4, 2021 11:11 AM
> To: jerinj@marvell.com; Jayatheerthan, Jay <jay.jayatheerthan@intel.com>
> Cc: dev@dpdk.org; Kundapura, Ganapati <ganapati.kundapura@intel.com>
> Subject: [PATCH v5 1/5] eventdev/rx_adapter: add event buffer size configurability
> 
> Currently event buffer is static array with a default size defined
> internally.
> 
> To configure event buffer size from application,
> ``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 size is
> rounded up for better buffer utilization and performance . 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>
> 
> ---
> v5:
> * reverted queue conf get unit test change
> 
> v4:
> * rebased with latest dpdk-next-eventdev branch
> * changed queue conf get unit test
> 
> v3:
> * updated documentation and code comments as per review comments.
> * updated new create api test case name with suitable one.
> 
> 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       | 98 +++++++++++++++++--
>  lib/eventdev/rte_event_eth_rx_adapter.h       | 41 +++++++-
>  lib/eventdev/version.map                      |  2 +
>  4 files changed, 140 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 ce23d8a474..8526aecf57 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 10491ca07b..606db241b8 100644
> --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> @@ -85,7 +85,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 */
> @@ -946,7 +948,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],
> @@ -972,7 +974,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) {
> @@ -2206,12 +2208,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;
> @@ -2226,6 +2231,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;
> 
> @@ -2273,11 +2279,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);

Do we need to align event_buf_size again here ? The caller seems to take care of it.

> +
> +	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;
> 
> @@ -2293,6 +2318,61 @@ 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;

Can initialize rxa_params in case if more fields get added in future that we don't assign here.

> +
> +	/* use default values for adapter params */
> +	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;
> +
> +	/* adjust event buff size with BATCH_SIZE used for fetching packets
> +	 * from NIC rx queues to get full buffer utilization and prevent
> +	 * unnecessary rollovers.
> +	 */
> +	rxa_params->event_buf_size = RTE_ALIGN(rxa_params->event_buf_size,
> +					       BATCH_SIZE);
> +	rxa_params->event_buf_size += BATCH_SIZE + BATCH_SIZE;

Please add brackets to be more explicit and readable.

> +
> +	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)
> @@ -2302,12 +2382,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);
> @@ -2336,6 +2418,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;
> 
> @@ -2711,6 +2794,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 470543e434..846ca569e9 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()
> @@ -37,7 +38,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
> @@ -257,6 +258,17 @@ 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.
> +	 * This value is rounded up for better buffer utilization
> +	 * and performance.
> +	 */
> +};
> +
>  /**
>   *
>   * Callback function invoked by the SW adapter before it continues
> @@ -357,6 +369,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 9f280160fa..7de18497a6 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