> 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
