> -----Original Message-----
> From: Mattias Rönnblom <mattias.ronnb...@ericsson.com>
> Sent: Wednesday, August 24, 2022 2:12 PM
> To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>; Mattias
> Rönnblom <hof...@lysator.liu.se>; Jerin Jacob Kollanukkaran
> <jer...@marvell.com>; Jay Jayatheerthan <jay.jayatheert...@intel.com>
> Cc: dev@dpdk.org; erik.g.carri...@intel.com; abhinandan.guj...@intel.com;
> timothy.mcdan...@intel.com; Shijith Thotton <sthot...@marvell.com>;
> hemant.agra...@nxp.com; nipun.gu...@nxp.com;
> harry.van.haa...@intel.com; lian...@liangbit.com;
> peter.mccar...@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, pbhagavat...@marvell.com wrote:
> >>> From: Pavan Nikhilesh <pbhagavat...@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 <pbhagavat...@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
> >>>
