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

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

> -----Original Message-----
> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Sent: Wednesday, August 24, 2022 2:12 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Mattias
> Rönnblom <hofors@lysator.liu.se>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Jay Jayatheerthan <jay.jayatheerthan@intel.com>
> Cc: dev@dpdk.org; erik.g.carrillo@intel.com; abhinandan.gujjar@intel.com;
> timothy.mcdaniel@intel.com; Shijith Thotton <sthotton@marvell.com>;
> hemant.agrawal@nxp.com; nipun.gupta@nxp.com;
> harry.van.haaren@intel.com; liangma@liangbit.com;
> peter.mccarthy@intel.com
> Subject: Re: [EXT] Re: [PATCH 1/3] eventdev: add element offset to event
> vector
> 
> On 2022-08-23 22:39, Pavan Nikhilesh Bhagavatula wrote:
> >> 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.
> >
> 
> The issue is that it doesn't clarify anything. Change the name, or
> change the semantics to fit the name, instead of explaining a poor name
> in a comment.
>

Names are always subjective and will confuse someone or the other.
But we can do our best to communicate the semantics, how about 
total_(elements|slots|lanes) and valid_(element|slot|lane)_offset.

I will send the next version once we agree upon the naming.

> >> 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.
> >
> 
> I think you should focus more on the end result, rather how easily you
> can get there. In my experience, in the long run, that's what pays off
> is to keep the design clean and reduce the overall complexity.
> 
> You don't think having a field called "nb_elem" which value doesn't
> represent the number of elements, but rather something else, is 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
> >>>