Re: [PATCH 1/3] eventdev: add element offset to event vector

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2022-08-16 17:49, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Add ``elem_offset:12`` bit field event vector structure
> the bits are taken from ``rsvd:15``.
> The element offset defines the offset into the vector array
> at which valid elements start.
> The valid elements count will be equal to nb_elem - elem_offset.
> 

I'm missing a rationale why this change is a good idea. (I can guess, 
but I think it's better to spell it out.)

> Update Rx/Tx adapter SW implementation to use elem_offset.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
>   lib/eventdev/rte_event_eth_rx_adapter.c | 1 +
>   lib/eventdev/rte_event_eth_tx_adapter.c | 7 ++++---
>   lib/eventdev/rte_eventdev.h             | 8 ++++++--
>   3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c b/lib/eventdev/rte_event_eth_rx_adapter.c
> index bf8741d2ea..bd72f9b845 100644
> --- a/lib/eventdev/rte_event_eth_rx_adapter.c
> +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
> @@ -855,6 +855,7 @@ rxa_init_vector(struct event_eth_rx_adapter *rx_adapter,
>   	vec->vector_ev->port = vec->port;
>   	vec->vector_ev->queue = vec->queue;
>   	vec->vector_ev->attr_valid = true;
> +	vec->vector_ev->elem_offset = 0;
>   	TAILQ_INSERT_TAIL(&rx_adapter->vector_list, vec, next);
>   }
> 
> diff --git a/lib/eventdev/rte_event_eth_tx_adapter.c b/lib/eventdev/rte_event_eth_tx_adapter.c
> index b4b37f1cae..da70883e0d 100644
> --- a/lib/eventdev/rte_event_eth_tx_adapter.c
> +++ b/lib/eventdev/rte_event_eth_tx_adapter.c
> @@ -524,16 +524,17 @@ txa_process_event_vector(struct txa_service_data *txa,
>   		queue = vec->queue;
>   		tqi = txa_service_queue(txa, port, queue);
>   		if (unlikely(tqi == NULL || !tqi->added)) {
> -			rte_pktmbuf_free_bulk(mbufs, vec->nb_elem);
> +			rte_pktmbuf_free_bulk(&mbufs[vec->elem_offset],
> +					      vec->nb_elem - vec->elem_offset);
>   			rte_mempool_put(rte_mempool_from_obj(vec), vec);
>   			return 0;
>   		}
> -		for (i = 0; i < vec->nb_elem; i++) {
> +		for (i = vec->elem_offset; i < vec->nb_elem; i++) {
>   			nb_tx += rte_eth_tx_buffer(port, queue, tqi->tx_buf,
>   						   mbufs[i]);
>   		}
>   	} else {
> -		for (i = 0; i < vec->nb_elem; i++) {
> +		for (i = vec->elem_offset; i < vec->nb_elem; i++) {
>   			port = mbufs[i]->port;
>   			queue = rte_event_eth_tx_adapter_txq_get(mbufs[i]);
>   			tqi = txa_service_queue(txa, port, queue);
> diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> index 6a6f6ea4c1..b0698fe748 100644
> --- a/lib/eventdev/rte_eventdev.h
> +++ b/lib/eventdev/rte_eventdev.h
> @@ -1060,8 +1060,12 @@ rte_event_dev_close(uint8_t dev_id);
>    */
>   struct rte_event_vector {
>   	uint16_t nb_elem;
> -	/**< Number of elements in this event vector. */
> -	uint16_t rsvd : 15;
> +	/**< Total number of elements in this event vector. */

I'm not sure "total" adds anything here. Didn't the old nb_elem also 
include the total number of elements?

nb_elem doesn't represent the number of elements in the vector any more, 
does it?

Why not just keep the old semantics, and let it represent the number of 
used slots in the vector array? As opposed to being the <last used 
index> + 1.

> +	uint16_t elem_offset : 12;
> +	/**< Offset into the vector array where valid elements start from.
> +	 * The valid elements count would be nb_elem - elem_offset.
> +	 */
> +	uint16_t rsvd : 3;
>   	/**< Reserved for future use */
>   	uint16_t attr_valid : 1;
>   	/**< Indicates that the below union attributes have valid information.
> --
> 2.25.1
>