[1/3] eventdev: add element offset to event vector
Checks
Commit Message
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.
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(-)
--
2.25.1
Comments
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
>
> 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
> >
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.
>> 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
>>>
> -----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
> >>>
> > >>> 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.
In order to make forward progress, @Mattias Rönnblom Do you have
specific name suggestions for the next version?
If not, I suggest to pick total_elements
On 2022-09-14 15:02, Jerin Jacob wrote:
>>>>>> 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.
>
> In order to make forward progress, @Mattias Rönnblom Do you have
> specific name suggestions for the next version?
> If not, I suggest to pick total_elements
I may be missing something here, but I would suggest keeping the old
name and old semantics. I.e, nb_elem is the number of elements actually
used. New is only that they may start at index elem_offset, as opposed
to the old always-at-index-0.
Instead of changes like:
- for (i = 0; i < vec->nb_elem; i++) {
+ for (i = vec->elem_offset; i < vec->nb_elem; i++) {
port = mbufs[i]->port;
queue =
You would have:
for (i = 0; i < vec->nb_elem; i++) {
- port = mbufs[i]->port;
+ port = mbufs[i + vec->elem_offset]->port;
queue =
If you for some reason want to have the start index and the end index
(like the patch suggested), you could replace the original nb_elem with
two fields, elem_start (what patch calls elem_offset) and elem_end (what
patch call nb_elem). I think having only an offset and a length is more
straightforward though. In the elem_end case, you will have people
asking themselves if it is the last index used, or the first index not
used (i.e., last index + 1).
@@ -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);
}
@@ -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);
@@ -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. */
+ 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.