Memory leak and “mbuf” duplicate release.

[daoxuans]

Q1: Memory leak DpdkDevice.cpp: L1112 MBufRawPacket* newRawPacket = new MBufRawPacket(); I can't find where to delete “newRawPacket”. e.g. in "Tests/Pcap++Test/main.cpp:L379~L395"

while (!m_Stop)
{
    pthread_mutex_lock(m_QueueLock);
    uint16_t packetReceived = m_DpdkDevice->receivePackets(mBufArr, 32, m_QueueId);
    pthread_mutex_unlock(m_QueueLock);
    m_PacketCount += packetReceived;
    pthread_mutex_lock(m_QueueLock);
    uint16_t packetsSent = m_DpdkDevice->sendPackets(mBufArr, packetReceived, m_QueueId);
    PCAPP_ASSERT(packetsSent == packetReceived, "Couldn't send all received packets on thread %d", m_CoreId);
    pthread_mutex_unlock(m_QueueLock);
}

for (int i = 0; i < 32; i++)
{
    if (mBufArr[i] != NULL)
        delete mBufArr[i];
}

Only the last 32 mBufArr[i] are released.

Q2: duplicate release DpdkDevice.cpp:L1278

uint16_t DpdkDevice::sendPackets(MBufRawPacket** rawPacketsArr, uint16_t arrLength, uint16_t txQueueId, bool useTxBuffer)
{
    uint16_t packetsSent = sendPacketsInner(txQueueId, (void*)rawPacketsArr, getNextPacketFromMBufRawPacketArray, arrLength, useTxBuffer);

    bool needToFreeMbuf = (!useTxBuffer && (packetsSent != arrLength));

    for (int index = 0; index < arrLength; index++)
        rawPacketsArr[index]->setFreeMbuf(needToFreeMbuf);

    return packetsSent;
}

this line "bool needToFreeMbuf = (!useTxBuffer && (packetsSent != arrLength));" is used to determine if it needs to be released. but if "packetsSent != arrLength" is true, this means that only some of the mbufs were sent. so "for (int index = 0; index < arrLength; index++)" is not correct, only those mbufs that have not sent successfully need to be released in the destructor.

[seladb]

Regarding Q1: this is not a bug. The receivePackets() method returns an array of MBufRawPacket which should be free by the user, please refer to the documentation for this method:

Notice it's the user responsibility to free the array and its content when done using it

Regarding Q2: Please refer to the documentation for this method:

The mbufs used in this method aren't freed by this method, they will be transparently freed by DPDK

I didn't want to make an exception if not all packets are sent by DPDK, that's why I decided to free all of them

[daoxuans]

@seladb Q1: I agree with your point of view, MBufRawPacket which allocated in libPcap++ and should be free by the user. But in "Tests/Pcap++Test/main.cpp:L379~L395", mBufArr inside while (!m_Stop) clause: was not release. is this correct？

[daoxuans]

@seladb Q2: The mbufs will be transparently freed by DPDK when the packets actually sent by "rte_eth_tx_burst()". Applications that implement a "send as many packets to transmit as possible" policy can check this specific case and keep invoking the rte_eth_tx_burst() function until a value less than nb_pkts is returned. so you can find some code segment like this in dpdk example: dpdk: examples/ip_fragmentation/main.c

/* Send burst of packets on an output interface */
static inline int
send_burst(struct lcore_queue_conf *qconf, uint16_t n, uint8_t port)
{
    struct rte_mbuf **m_table;
    int ret;
    uint16_t queueid;
    queueid = qconf->tx_queue_id[port];
    m_table = (struct rte_mbuf **)qconf->tx_mbufs[port].m_table;
    ret = rte_eth_tx_burst(port, queueid, m_table, n);
    if (unlikely(ret < n)) {
            do {
                    rte_pktmbuf_free(m_table[ret]);
            } while (++ret < n);
    }
    return 0;
}

you must manually release the mbuf that failed to send.

[daoxuans]

@seladb My understanding is wrong. Repeated release is allowed, even if a successfully sent mbuf can be released again. One is a transparent release(rte_eth_tx_burst) and the other is a manual release(rte_pktmbuf_free).

[seladb]

Regarding Q1: I agree that the unit-test code is wrong. I'll fix that when I have the time, but I don't think it's very urgent. Please let me know me if you disagree

Regarding Q2: this code can indeed be changed to release just the mbufs that failed to be sent, but as you mentioned, double release is also allowed. What do you think?

[daoxuans]

Regarding Q2: In my test, repeated release will generate some extra consumption, which becomes more obvious in large traffic scenarios.
Benchmark: NIC: one card, Intel X520-DA2(10GbE) Server: Dell PowerEdge R930 Traffic： Ingress Port >7Gbps

[seladb]

Sure. Do you have the time and capacity to do this fix and create a PR?

[seladb]

I was wrong in my answer on Q1: the unit-test code is actually fine: this while loop receives packets into an array and immediately sends them. The send method should take care of freeing all the mbufs, so there is nothing else to free and there should be no memory leak. Please let me know if you have other observation

In the meantime I'll fix only the issue you raised in Q2

[seladb]

There is no need to free the elements (of type MBufRawPacket) in mBufArr. They will be reused in each iteration of the loop. The sendPackets() method will just free the mbufs inside this elements, it won't create them again if they already exist.

Please let me know what you think

On Wed, Jun 27, 2018 at 12:58 AM Daoxuans notifications@github.com wrote:

DpdkDevice.cpp: L1110

for (size_t index = 0; index < packetsReceived; ++index) { struct rte_mbuf* mBuf = mBufArray[index]; if (rawPacketsArr[index] == NULL) rawPacketsArr[index] = new MBufRawPacket();

rawPacketsArr[index]->setMBuf(mBuf, time); }

Each time this API(uint16_t DpdkDevice::receivePackets(MBufRawPacket** rawPacketsArr, ...) is called, multiple MBufRawPacket class objects are allocated. But where do you release the class object allocated here?

My changes are as follows in "Tests/Pcap++Test/main.cpp:L379~L395".

while (!m_Stop) { pthread_mutex_lock(m_QueueLock); uint16_t packetReceived = m_DpdkDevice->receivePackets(mBufArr, 32, m_QueueId); pthread_mutex_unlock(m_QueueLock); m_PacketCount += packetReceived; pthread_mutex_lock(m_QueueLock); uint16_t packetsSent = m_DpdkDevice->sendPackets(mBufArr, packetReceived, m_QueueId); PCAPP_ASSERT(packetsSent == packetReceived, "Couldn't send all received packets on thread %d", m_CoreId); pthread_mutex_unlock(m_QueueLock); for (int i = 0; i < 32; i++) { if (mBufArr[i] != NULL) { delete mBufArr[i]; mBufArr[i] = NULL； } } }

PCAPP_DEBUG_PRINT("Worker thread on %d stopped", m_CoreId);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/seladb/PcapPlusPlus/issues/121#issuecomment-400578953, or mute the thread https://github.com/notifications/unsubscribe-auth/AIo81UR0GpBuRhne0sCB8N0TuT3PuiApks5uAzsggaJpZM4U3P7i .

[seladb]

Fixed the double release issue. Please verify the code and reopen this issue if you find any problem