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

[Mattias Rönnblom]

On 2022-08-23 22:39, Pavan Nikhilesh Bhagavatula wrote:
>> On 2022-08-16 17:49, pbhagavatula at marvell.com wrote:
>>> From: Pavan Nikhilesh <pbhagavatula at 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 at 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
>>>