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

[Long Li <longli@microsoft.com>]

> Subject: Re: [Patch v2] net/mana: use rte_pktmbuf_alloc_bulk for allocating RX
> WQEs
> 
> 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.

Will do that.

> 
> >>
> >> <...>
> >>
> >>> @@ -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()'.

You are right, I'm sending v3 to fix those.

Long