Re: [Patch v2] net/mana: use rte_pktmbuf_alloc_bulk for allocating RX WQEs

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 1/30/2024 9:30 PM, Long Li wrote:
>> Can you please quantify the performance improvement (as percentage), this
>> clarifies the impact of the modification.
> 
> I didn't see any meaningful performance improvements in benchmarks. However, this should improve CPU cycles and reduce potential locking conflicts in real-world applications. 
> 
> Using batch allocation was one of the review comments during initial driver submission, suggested by Stephen Hemminger. I promised to fix it at that time. Sorry it took a while to submit this patch.
> 

That is OK, using bulk alloc is reasonable approach, only can you please
document the impact (performance increase) in the commit log.

>>
>> <...>
>>
>>> @@ -121,19 +115,32 @@ mana_alloc_and_post_rx_wqe(struct mana_rxq
>> *rxq)
>>>   * Post work requests for a Rx queue.
>>>   */
>>>  static int
>>> -mana_alloc_and_post_rx_wqes(struct mana_rxq *rxq)
>>> +mana_alloc_and_post_rx_wqes(struct mana_rxq *rxq, uint32_t count)
>>>  {
>>>  	int ret;
>>>  	uint32_t i;
>>> +	struct rte_mbuf **mbufs;
>>> +
>>> +	mbufs = rte_calloc_socket("mana_rx_mbufs", count, sizeof(struct
>> rte_mbuf *),
>>> +				  0, rxq->mp->socket_id);
>>> +	if (!mbufs)
>>> +		return -ENOMEM;
>>>
>>
>> 'mbufs' is temporarily storage for allocated mbuf pointers, why not allocate if from
>> stack instead, can be faster and easier to manage:
>> "struct rte_mbuf *mbufs[count]"
>>
>>
>>> +
>>> +	ret = rte_pktmbuf_alloc_bulk(rxq->mp, mbufs, count);
>>> +	if (ret) {
>>> +		DP_LOG(ERR, "failed to allocate mbufs for RX");
>>> +		rxq->stats.nombuf += count;
>>> +		goto fail;
>>> +	}
>>>
>>>  #ifdef RTE_ARCH_32
>>>  	rxq->wqe_cnt_to_short_db = 0;
>>>  #endif
>>> -	for (i = 0; i < rxq->num_desc; i++) {
>>> -		ret = mana_alloc_and_post_rx_wqe(rxq);
>>> +	for (i = 0; i < count; i++) {
>>> +		ret = mana_post_rx_wqe(rxq, mbufs[i]);
>>>  		if (ret) {
>>>  			DP_LOG(ERR, "failed to post RX ret = %d", ret);
>>> -			return ret;
>>> +			goto fail;
>>>
>>
>> This may leak memory. There are allocated mbufs, if exit from loop here and free
>> 'mubfs' variable, how remaining mubfs will be freed?
> 
> Mbufs are always freed after fail:
> 
> fail:
>         rte_free(mbufs);
> 

Nope, I am not talking about the 'mbufs' variable, I am talking about
mbuf pointers stored in the 'mbufs' array which are allocated by
'rte_pktmbuf_alloc_bulk()'.