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