Re: [dpdk-dev] Question: Can't make pcap and refcnt to match

[Mats Liljegren <liljegren.mats2@gmail.com>]

Hi Bruce,

I made a dead simple patch that seems to fix my problem. Could you
check and see whether I'm on the right track here?

The patch is as follows:

-- >8 --

>From 901083b82c0e07f2535ee13f90e1a68c0f96602a Mon Sep 17 00:00:00 2001
From: Mats Liljegren <mats.liljegren@enea.com>
Date: Tue, 3 Dec 2013 17:56:01 +0100
Subject: [PATCH] Fixed bug with receive causing segmentation violation and
 lost buffers

A static list of 64 mbufs was being reused. This caused two errors:
1) If more than 64 buffer were requested in a single burst,
   only the last 64 buffers are returned, the others are lost.
2) Application will free the mbuf being returned, but the receive
   function will reuse the buffer anyway. If some other allocation
   is done there is suddenly multiple writers for same mbuf.

The fix consists of replacing the reused buffers with an allocation
for each buffer being returned.

Signed-off-by: Mats Liljegren <mats.liljegren@enea.com>
---
 lib/librte_pmd_pcap/rte_eth_pcap.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c
b/lib/librte_pmd_pcap/rte_eth_pcap.c
index 19d19b3..08a944c 100644
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -118,30 +118,28 @@ eth_pcap_rx(void *queue,
        struct pcap_pkthdr header;
        const u_char *packet;
        struct rte_mbuf *mbuf;
-       static struct rte_mbuf *mbufs[RTE_ETH_PCAP_MBUFS] = { 0 };
        struct pcap_rx_queue *pcap_q = queue;
        uint16_t num_rx = 0;

        if (unlikely(pcap_q->pcap == NULL || nb_pkts == 0))
                return 0;

-       if(unlikely(!mbufs[0]))
-               for (i = 0; i < RTE_ETH_PCAP_MBUFS; i++)
-                       mbufs[i] = rte_pktmbuf_alloc(pcap_q->mb_pool);
-
        /* Reads the given number of packets from the pcap file one by one
         * and copies the packet data into a newly allocated mbuf to return.
         */
        for (i = 0; i < nb_pkts; i++) {
-               mbuf = mbufs[i % RTE_ETH_PCAP_MBUFS];
+               char * msg;
                packet = pcap_next(pcap_q->pcap, &header);
                if (unlikely(packet == NULL))
                        break;
+               mbuf = rte_pktmbuf_alloc(pcap_q->mb_pool);
                if (unlikely(mbuf == NULL))
                        break;
-               rte_memcpy(mbuf->pkt.data, packet, header.len);
-               mbuf->pkt.data_len = (uint16_t)header.len;
-               mbuf->pkt.pkt_len = mbuf->pkt.data_len;
+               msg = rte_pktmbuf_append(mbuf, header.len);
+               if (unlikely(msg == NULL)) {
+                       rte_panic("MBuf too small, needed %u bytes\n",
header.len);
+               }
+               rte_memcpy(msg, packet, header.len);
                bufs[i] = mbuf;
                num_rx++;
        }
-- 
1.8.3.2


On Tue, Nov 26, 2013 at 2:46 PM, Richardson, Bruce
<bruce.richardson@intel.com> wrote:
> Hi Mats,
>
> yes, you are right, there is an issue in the pcap driver that it is not allocating mbufs correctly. We are working on a fix.
>
> Regards,
> /Bruce
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mats Liljegren
>> Sent: Tuesday, November 26, 2013 1:07 PM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] Question: Can't make pcap and refcnt to match
>>
>> I have had stability problems when using pcap in my little application. My
>> application is a simple benchmark applications that is trying to see how
>> much data I can send and receive.
>>
>> It has one lcore per NIC, where each lcore handles transmit and receive. On
>> the hardware, I make a loopback between two NICs, so the NICs are in
>> practice paired. I currently use 4 NICs and therefore 4 lcores. Port 0 sends to
>> port 1 and vice versa. Port 2 send to port 3 and vice versa. One pair is using
>> DPDK hardware driver against a dual
>> i350 NIC. The other pair is using pcap against two of the four on-board NICs.
>>
>> When enabling everything saying "DEBUG" in its name in the .config file, I
>> get the following error:
>>
>> PMD: rte_eth_dev_config_restore: port 1: MAC address array not
>> supported
>> PMD: rte_eth_promiscuous_disable: Function not supported
>> PMD: rte_eth_allmulticast_disable: Function not supported
>> Speed: 10000 Mbps, full duplex
>> Port 1 up and running.
>> PMD: e1000_put_hw_semaphore_generic():
>> e1000_put_hw_semaphore_generic PANIC in rte_mbuf_sanity_check():
>> bad ref cnt
>> PANIC in rte_mbuf_sanity_check():
>> bad ref cnt
>> PMD: e1000_release_phy_82575(): e1000_release_phy_82575
>> PMD: e1000_release_swfw_sync_82575():
>> e1000_release_swfw_sync_82575
>> PMD: e1000_get_hw_semaphore_generic():
>> e1000_get_hw_semaphore_generic
>> PMD: eth_igb_rx_queue_setup(): sw_ring=0x7fff776eefc0
>> hw_ring=0x7fff76830480 dma_addr=0x464630480
>>
>> PMD: e1000_put_hw_semaphore_generic():
>> e1000_put_hw_semaphore_generic
>> PMD: To improve 1G driver performance, consider setting the TX WTHRESH
>> value to 4, 8, or 16.
>> PMD: eth_igb_tx_queue_setup(): sw_ring=0x7fff776ece40
>> hw_ring=0x7fff76840500 dma_addr=0x464640500
>>
>> PMD: eth_igb_start(): >>
>> PMD: e1000_read_phy_reg_82580(): e1000_read_phy_reg_82580
>> PMD: e1000_acquire_phy_82575(): e1000_acquire_phy_82575
>> PMD: e1000_acquire_swfw_sync_82575():
>> e1000_acquire_swfw_sync_82575
>> PMD: e1000_get_hw_semaphore_generic():
>> e1000_get_hw_semaphore_generic
>> PMD: e1000_get_cfg_done_82575(): e1000_get_cfg_done_82575
>> PMD: e1000_put_hw_semaphore_generic():
>> e1000_put_hw_semaphore_generic
>> PMD: e1000_read_phy_reg_mdic(): e1000_read_phy_reg_mdic
>> 9: [/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d) [0x7ffff72a89cd]]
>> 8: [/lib/x86_64-linux-gnu/libpthread.so.0(+0x7f6e) [0x7ffff757df6e]]
>> 7: [/home/mlil/dpdk-demo/build/enea-demo(eal_thread_loop+0x1b9)
>> [0x492669]]
>> 6: [/home/mlil/dpdk-demo/build/enea-demo() [0x4150bc]]
>> 5: [/home/mlil/dpdk-demo/build/enea-demo() [0x414d0b]]
>> 4: [/home/mlil/dpdk-demo/build/enea-demo() [0x4116ef]]
>> 3: [/home/mlil/dpdk-demo/build/enea-
>> demo(rte_mbuf_sanity_check+0xa7) [0x484707]]
>> 2: [/home/mlil/dpdk-demo/build/enea-demo(__rte_panic+0xc1)
>> [0x40f788]]
>> 1: [/home/mlil/dpdk-demo/build/enea-demo(rte_dump_stack+0x18)
>> [0x493f68]]
>> PMD: e1000_release_phy_82575(): e1000_release_phy_82575
>> PMD: e1000_release_swfw_sync_82575():
>> e1000_release_swfw_sync_82575
>> PMD: e1000_get_hw_semaphore_generic():
>> e1000_get_hw_semaphore_generic
>>
>> I checked the source code for pcap, and in the file rte_eth_pcap.c, function
>> eth_pcap_rx(), I make the following observation:
>>
>> It pre-allocates a number of mbufs (64 to be exact). It then fills these mbufs
>> with data and returns them. The pre-allocation seems to only be done once,
>> and then they are re-used.
>>
>> This confuses me. How does this work when more than 64 packets are
>> requested? I see no safety checks for this.
>>
>> Aren't application supposed to call rte_pktmbuf_free() on the returned
>> mbufs? If so, the pre-allocated mbufs will have been free'd as far as I can
>> see and can therefore not be re-used.
>>
>> What am I missing here?
>>
>> Regards
>> Mats