* Re: [dpdk-dev] Question: Can't make pcap and refcnt to match
2013-11-26 13:46 ` Richardson, Bruce
@ 2013-11-26 15:42 ` Robert Sanford
2013-11-26 16:05 ` Thomas Monjalon
2013-12-03 17:04 ` Mats Liljegren
1 sibling, 1 reply; 7+ messages in thread
From: Robert Sanford @ 2013-11-26 15:42 UTC (permalink / raw)
To: Richardson, Bruce, liljegren.mats2, dev
Hi Bruce,
We also found buffer overflow problems with the pcap driver. 1) Frame may
be longer than mbuf. 2) Caplen may be less than original packet.
I've been meaning to submit a change, but I'm not familiar with the process.
Here is a diff of the relevant code in rte_eth_pcap.c:
====
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;
+
+ /*
+ * Fix buffer overflow problems.
+ * 1. Frame may be longer than mbuf.
+ * 2. Capture length (caplen) may be less than original packet length.
+ */
+ uint16_t len = (uint16_t)header.caplen;
+ uint16_t tailroom = rte_pktmbuf_tailroom(mbuf);
+ if (len > tailroom)
+ len = tailroom;
+
+ /****
+ RTE_LOG(INFO, PMD, "eth_pcap_rx: i=%u caplen=%u framelen=%u tail=%u
len=%u\n",
+ i, header.caplen, header.len, tailroom, len);
+ ****/
+
+ rte_memcpy(mbuf->pkt.data, packet, len);
+ mbuf->pkt.data_len = len;
+ mbuf->pkt.pkt_len = len;
+
bufs[i] = mbuf;
num_rx++;
}
====
Regards,
Robert
On Tue, Nov 26, 2013 at 8:46 AM, 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] Question: Can't make pcap and refcnt to match
2013-11-26 13:46 ` Richardson, Bruce
2013-11-26 15:42 ` Robert Sanford
@ 2013-12-03 17:04 ` Mats Liljegren
2013-12-04 10:44 ` Richardson, Bruce
1 sibling, 1 reply; 7+ messages in thread
From: Mats Liljegren @ 2013-12-03 17:04 UTC (permalink / raw)
To: Richardson, Bruce; +Cc: dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread