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

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

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

Sure, I will add it in the next version.

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

Yes, I added it to clarify that it includes slots that don’t have valid elements.
I will update the comment to convey that it includes elements before offset.

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

I think its simpler to just manage updates to the vector by updating elem_offset and keeping 
nb_elem as a constant, valid elements count can simply be calculated via nb_elem - elem_offset.
Vector is empty when nb_elem = elem_offset and can be reused simply by setting elem_offset to 0.

Having to update both nb_elem and elem_offset might be a tad bit error prone.

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