[dpdk-dev] [PATCH 0/3] pcap pmd improvements

[dpdk-dev] [PATCH 0/3] pcap pmd improvements

[Tero Aho]

We have found it convenient to use pcap interfaces in early development
when speed does not matter. However, pcap interfaces use fixed values
for some properties like link status which makes it hard to simulate
certain conditions.

Here's series of small improvements we have originally used on top
of v1.7.0 (now rebased to current master).

Tero Aho (3):
  pcap: utilize underlying real interface properties
  pcap: add support for jumbo frames
  pcap: add byte and error counters into statistics

 lib/librte_pmd_pcap/rte_eth_pcap.c | 176 +++++++++++++++++++++++++++++++------
 1 file changed, 151 insertions(+), 25 deletions(-)

--
1.9.1


============================================================
The information contained in this message may be privileged
and confidential and protected from disclosure. If the reader
of this message is not the intended recipient, or an employee
or agent responsible for delivering this message to the
intended recipient, you are hereby notified that any reproduction,
dissemination or distribution of this communication is strictly
prohibited. If you have received this communication in error,
please notify us immediately by replying to the message and
deleting it from your computer. Thank you. Coriant-Tellabs
============================================================

[dpdk-dev] [PATCH 1/3] pcap: utilize underlying real interface properties

[Tero Aho]

These changes set pcap interface mac address to the real underlying
interface address instead of the default one. Also real interface link
status, speed and duplex are reported when eth_link_update is called
for the pcap interface.

Signed-off-by: Tero Aho <tero.aho@coriant.com>
---
 lib/librte_pmd_pcap/rte_eth_pcap.c | 51 +++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c
index 5e94930..289af28 100644
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -43,6 +43,11 @@
 #include <rte_dev.h>

 #include <net/if.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
+#include <string.h>
+#include <linux/ethtool.h>
+#include <linux/sockios.h>

 #include <pcap.h>

@@ -102,6 +107,8 @@ struct pmd_internals {
        unsigned nb_tx_queues;
        int if_index;
        int single_iface;
+       const char *if_name;
+       int if_fd;
 };

 const char *valid_arguments[] = {
@@ -451,6 +458,26 @@ static int
 eth_link_update(struct rte_eth_dev *dev __rte_unused,
                int wait_to_complete __rte_unused)
 {
+       struct ifreq ifr;
+       struct ethtool_cmd cmd;
+       struct pmd_internals *internals = dev->data->dev_private;
+
+       if (internals->if_name && (internals->if_fd != -1)) {
+               /* get link status, speed and duplex from the underlying interface */
+
+               strncpy(ifr.ifr_name, internals->if_name, sizeof(ifr.ifr_name)-1);
+               ifr.ifr_name[sizeof(ifr.ifr_name)-1] = 0;
+               if (!ioctl(internals->if_fd, SIOCGIFFLAGS, &ifr))
+                       dev->data->dev_link.link_status = (ifr.ifr_flags & IFF_UP) ? 1 : 0;
+
+               cmd.cmd = ETHTOOL_GSET;
+               ifr.ifr_data = (void *)&cmd;
+               if (!ioctl(internals->if_fd, SIOCETHTOOL, &ifr)) {
+                       dev->data->dev_link.link_speed = ethtool_cmd_speed(&cmd);
+                       dev->data->dev_link.link_duplex =
+                               cmd.duplex ? ETH_LINK_FULL_DUPLEX : ETH_LINK_HALF_DUPLEX;
+               }
+       }
        return 0;
 }

@@ -736,11 +763,24 @@ rte_pmd_init_internals(const char *name, const unsigned nb_rx_queues,
        (*internals)->nb_rx_queues = nb_rx_queues;
        (*internals)->nb_tx_queues = nb_tx_queues;

-       if (pair == NULL)
+       if (pair == NULL) {
                (*internals)->if_index = 0;
-       else
+       } else {
+               /* use real inteface mac addr, save name and fd for eth_link_update */
                (*internals)->if_index = if_nametoindex(pair->value);
-
+               (*internals)->if_name = strdup(pair->value);
+               (*internals)->if_fd = socket(AF_INET, SOCK_DGRAM, 0);
+               if ((*internals)->if_fd != -1) {
+                       struct ifreq ifr;
+                       strncpy(ifr.ifr_name, pair->value, sizeof(ifr.ifr_name)-1);
+                       ifr.ifr_name[sizeof(ifr.ifr_name)-1] = 0;
+                       if (!ioctl((*internals)->if_fd, SIOCGIFHWADDR, &ifr)) {
+                               data->mac_addrs = rte_zmalloc_socket(NULL, ETHER_ADDR_LEN, 0, numa_node);
+                               if (data->mac_addrs)
+                                       rte_memcpy(data->mac_addrs, ifr.ifr_addr.sa_data, ETHER_ADDR_LEN);
+                       }
+               }
+       }
        pci_dev->numa_node = numa_node;

        data->dev_private = *internals;
@@ -749,7 +789,8 @@ rte_pmd_init_internals(const char *name, const unsigned nb_rx_queues,
        data->nb_rx_queues = (uint16_t)nb_rx_queues;
        data->nb_tx_queues = (uint16_t)nb_tx_queues;
        data->dev_link = pmd_link;
-       data->mac_addrs = &eth_addr;
+       if (data->mac_addrs == NULL)
+               data->mac_addrs = &eth_addr;
        strncpy(data->name,
                (*eth_dev)->data->name, strlen((*eth_dev)->data->name));

@@ -758,6 +799,8 @@ rte_pmd_init_internals(const char *name, const unsigned nb_rx_queues,
        (*eth_dev)->pci_dev = pci_dev;
        (*eth_dev)->driver = &rte_pcap_pmd;

+       eth_link_update((*eth_dev), 0);
+
        return 0;

        error: if (data)
--
1.9.1


============================================================
The information contained in this message may be privileged
and confidential and protected from disclosure. If the reader
of this message is not the intended recipient, or an employee
or agent responsible for delivering this message to the
intended recipient, you are hereby notified that any reproduction,
dissemination or distribution of this communication is strictly
prohibited. If you have received this communication in error,
please notify us immediately by replying to the message and
deleting it from your computer. Thank you. Coriant-Tellabs
============================================================

[dpdk-dev] [PATCH 2/3] pcap: add support for jumbo frames

[Tero Aho]

Extend eth_pcap_rx and eth_pcap_tx to support jumbo frames.
On receive side read large packets into multiple mbufs and
on the transmit side do the opposite.

Signed-off-by: Tero Aho <tero.aho@coriant.com>
---
 lib/librte_pmd_pcap/rte_eth_pcap.c | 88 ++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 13 deletions(-)

diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c
index 289af28..3f23f0a 100644
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -52,7 +52,7 @@
 #include <pcap.h>

 #define RTE_ETH_PCAP_SNAPSHOT_LEN 65535
-#define RTE_ETH_PCAP_SNAPLEN 4096
+#define RTE_ETH_PCAP_SNAPLEN ETHER_MAX_JUMBO_FRAME_LEN
 #define RTE_ETH_PCAP_PROMISC 1
 #define RTE_ETH_PCAP_TIMEOUT -1
 #define ETH_PCAP_RX_PCAP_ARG  "rx_pcap"
@@ -132,6 +132,43 @@ static struct rte_eth_link pmd_link = {
                .link_status = 0
 };

+/*
+ * Helper function to read mbuf chain.
+ */
+static int
+eth_pcap_rx_jumbo(struct rte_mempool *mb_pool,
+                 struct rte_mbuf *mbuf,
+                 const u_char *data,
+                 uint16_t data_len)
+{
+       struct rte_mbuf *m = mbuf;
+       /* copy first segment */
+       uint16_t len = rte_pktmbuf_tailroom(mbuf);
+       rte_memcpy(rte_pktmbuf_append(mbuf, len), data, len);
+       data_len -= len;
+       data += len;
+
+       while (data_len > 0) {
+               /* allocate next mbuf and point to that */
+               m->next = rte_pktmbuf_alloc(mb_pool);
+               if (unlikely(m->next == NULL))
+                       return -1;
+               m = m->next;
+
+               /* no headroom needed in chained mbufs */
+               rte_pktmbuf_prepend(m, rte_pktmbuf_headroom(m));
+               m->data_len = m->pkt_len = 0;
+
+               /* copy next segment */
+               len = RTE_MIN(rte_pktmbuf_tailroom(m), data_len);
+               rte_memcpy(rte_pktmbuf_append(m, len), data, len);
+
+               mbuf->nb_segs++;
+               data_len -= len;
+               data += len;
+       }
+       return mbuf->nb_segs;
+}

 static uint16_t
 eth_pcap_rx(void *queue,
@@ -173,17 +210,18 @@ eth_pcap_rx(void *queue,
                        rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
                                        header.len);
                        mbuf->data_len = (uint16_t)header.len;
-                       mbuf->pkt_len = mbuf->data_len;
-                       mbuf->port = pcap_q->in_port;
-                       bufs[num_rx] = mbuf;
-                       num_rx++;
                } else {
-                       /* pcap packet will not fit in the mbuf, so drop packet */
-                       RTE_LOG(ERR, PMD,
-                                       "PCAP packet %d bytes will not fit in mbuf (%d bytes)\n",
-                                       header.len, buf_size);
-                       rte_pktmbuf_free(mbuf);
+                       /* pcap packet will not fit in a single mbuf */
+                       if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool, mbuf,
+                                       packet, header.len) == -1)) {
+                               rte_pktmbuf_free(mbuf);
+                               break;
+                       }
                }
+               mbuf->pkt_len = (uint16_t)header.len;
+               mbuf->port = pcap_q->in_port;
+               bufs[num_rx] = mbuf;
+               num_rx++;
        }
        pcap_q->rx_pkts += num_rx;
        return num_rx;
@@ -241,6 +279,27 @@ eth_pcap_tx_dumper(void *queue,
 }

 /*
+ * Helper function to write mbuf chain.
+ */
+static uint16_t
+eth_pcap_tx_jumbo(pcap_t *pcap,
+                 struct rte_mbuf *mbuf)
+{
+       u_char data[ETHER_MAX_JUMBO_FRAME_LEN];
+       uint16_t data_len = 0;
+
+       while (mbuf != NULL) {
+               /* There is no writev style function in libpcap, */
+               /* we unfortunately have to copy data to a buffer. */
+               rte_memcpy(data + data_len, rte_pktmbuf_mtod(mbuf, void *),
+                          mbuf->data_len);
+               data_len += mbuf->data_len;
+               mbuf = mbuf->next;
+       }
+       return pcap_sendpacket(pcap, data, data_len);
+}
+
+/*
  * Callback to handle sending packets through a real NIC.
  */
 static uint16_t
@@ -259,9 +318,12 @@ eth_pcap_tx(void *queue,

        for (i = 0; i < nb_pkts; i++) {
                mbuf = bufs[i];
-               ret = pcap_sendpacket(tx_queue->pcap,
-                               rte_pktmbuf_mtod(mbuf, u_char *),
-                               mbuf->data_len);
+               if (likely(mbuf->nb_segs == 1))
+                       ret = pcap_sendpacket(tx_queue->pcap,
+                                             rte_pktmbuf_mtod(mbuf, u_char *),
+                                             mbuf->data_len);
+               else
+                       ret = eth_pcap_tx_jumbo(tx_queue->pcap, mbuf);
                if (unlikely(ret != 0))
                        break;
                num_tx++;
--
1.9.1


============================================================
The information contained in this message may be privileged
and confidential and protected from disclosure. If the reader
of this message is not the intended recipient, or an employee
or agent responsible for delivering this message to the
intended recipient, you are hereby notified that any reproduction,
dissemination or distribution of this communication is strictly
prohibited. If you have received this communication in error,
please notify us immediately by replying to the message and
deleting it from your computer. Thank you. Coriant-Tellabs
============================================================

[dpdk-dev] [PATCH 3/3] pcap: add byte and error counters into statistics

[Tero Aho]

Added input/ouput byte counters into pcap interface statistics,
also calls pcap_stats to add dropped packets into input errors.

Signed-off-by: Tero Aho <tero.aho@coriant.com>
---
 lib/librte_pmd_pcap/rte_eth_pcap.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c
index 3f23f0a..1c472ac 100644
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
@@ -71,6 +71,7 @@ struct pcap_rx_queue {
        uint8_t in_port;
        struct rte_mempool *mb_pool;
        volatile unsigned long rx_pkts;
+       volatile unsigned long rx_octs;
        volatile unsigned long err_pkts;
        const char *name;
        const char *type;
@@ -80,6 +81,7 @@ struct pcap_tx_queue {
        pcap_dumper_t *dumper;
        pcap_t *pcap;
        volatile unsigned long tx_pkts;
+       volatile unsigned long tx_octs;
        volatile unsigned long err_pkts;
        const char *name;
        const char *type;
@@ -218,6 +220,7 @@ eth_pcap_rx(void *queue,
                                break;
                        }
                }
+               pcap_q->rx_octs += header.len;
                mbuf->pkt_len = (uint16_t)header.len;
                mbuf->port = pcap_q->in_port;
                bufs[num_rx] = mbuf;
@@ -261,6 +264,7 @@ eth_pcap_tx_dumper(void *queue,
                calculate_timestamp(&header.ts);
                header.len = mbuf->data_len;
                header.caplen = header.len;
+               dumper_q->tx_octs += header.len;
                pcap_dump((u_char *)dumper_q->dumper, &header,
                                rte_pktmbuf_mtod(mbuf, void*));
                rte_pktmbuf_free(mbuf);
@@ -324,9 +328,11 @@ eth_pcap_tx(void *queue,
                                              mbuf->data_len);
                else
                        ret = eth_pcap_tx_jumbo(tx_queue->pcap, mbuf);
+
                if (unlikely(ret != 0))
                        break;
                num_tx++;
+               tx_queue->tx_octs += mbuf->pkt_len;
                rte_pktmbuf_free(mbuf);
        }

@@ -471,26 +477,38 @@ eth_stats_get(struct rte_eth_dev *dev,
                struct rte_eth_stats *igb_stats)
 {
        unsigned i;
-       unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
+       struct pcap_stat ps;
+       unsigned long ipackets = 0, ibytes = 0, ierrors = 0;
+       unsigned long opackets = 0, obytes = 0, oerrors = 0;
        const struct pmd_internals *internal = dev->data->dev_private;

        for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_rx_queues;
                        i++) {
                igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
-               rx_total += igb_stats->q_ipackets[i];
+               igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_octs;
+               if (!pcap_stats(internal->rx_queue[i].pcap, &ps))
+                       igb_stats->q_errors[i] = ps.ps_drop;
+               ipackets += igb_stats->q_ipackets[i];
+               ibytes += igb_stats->q_ibytes[i];
+               ierrors += igb_stats->q_errors[i];
        }

        for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS && i < internal->nb_tx_queues;
                        i++) {
                igb_stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
+               igb_stats->q_obytes[i] = internal->tx_queue[i].tx_octs;
                igb_stats->q_errors[i] = internal->tx_queue[i].err_pkts;
-               tx_total += igb_stats->q_opackets[i];
-               tx_err_total += igb_stats->q_errors[i];
+               opackets += igb_stats->q_opackets[i];
+               obytes += igb_stats->q_obytes[i];
+               oerrors += igb_stats->q_errors[i];
        }

-       igb_stats->ipackets = rx_total;
-       igb_stats->opackets = tx_total;
-       igb_stats->oerrors = tx_err_total;
+       igb_stats->ipackets = ipackets;
+       igb_stats->ibytes = ibytes;
+       igb_stats->ierrors = ierrors;
+       igb_stats->opackets = opackets;
+       igb_stats->obytes = obytes;
+       igb_stats->oerrors = oerrors;
 }

 static void
@@ -498,10 +516,13 @@ eth_stats_reset(struct rte_eth_dev *dev)
 {
        unsigned i;
        struct pmd_internals *internal = dev->data->dev_private;
-       for (i = 0; i < internal->nb_rx_queues; i++)
+       for (i = 0; i < internal->nb_rx_queues; i++) {
                internal->rx_queue[i].rx_pkts = 0;
+               internal->rx_queue[i].rx_octs = 0;
+       }
        for (i = 0; i < internal->nb_tx_queues; i++) {
                internal->tx_queue[i].tx_pkts = 0;
+               internal->tx_queue[i].tx_octs = 0;
                internal->tx_queue[i].err_pkts = 0;
        }
 }
--
1.9.1


============================================================
The information contained in this message may be privileged
and confidential and protected from disclosure. If the reader
of this message is not the intended recipient, or an employee
or agent responsible for delivering this message to the
intended recipient, you are hereby notified that any reproduction,
dissemination or distribution of this communication is strictly
prohibited. If you have received this communication in error,
please notify us immediately by replying to the message and
deleting it from your computer. Thank you. Coriant-Tellabs
============================================================

Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements

[Thomas Monjalon]

2015-02-27 15:42, Tero Aho:
> We have found it convenient to use pcap interfaces in early development
> when speed does not matter. However, pcap interfaces use fixed values
> for some properties like link status which makes it hard to simulate
> certain conditions.
> 
> Here's series of small improvements we have originally used on top
> of v1.7.0 (now rebased to current master).
> 
> Tero Aho (3):
>   pcap: utilize underlying real interface properties
>   pcap: add support for jumbo frames
>   pcap: add byte and error counters into statistics

Thanks for contributing your improvements!

> ============================================================
> The information contained in this message may be privileged
> and confidential and protected from disclosure. If the reader
> of this message is not the intended recipient, or an employee
> or agent responsible for delivering this message to the
> intended recipient, you are hereby notified that any reproduction,
> dissemination or distribution of this communication is strictly
> prohibited. If you have received this communication in error,
> please notify us immediately by replying to the message and
> deleting it from your computer. Thank you. Coriant-Tellabs
> ============================================================

You should ask your administrator to not add this footer when sending
to dpdk.org.
Thanks

Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements

[Thomas Monjalon]

Please Nicolas, John,
could we have a review of these patches?
Thanks

2015-02-27 15:42, Tero Aho:
> We have found it convenient to use pcap interfaces in early development
> when speed does not matter. However, pcap interfaces use fixed values
> for some properties like link status which makes it hard to simulate
> certain conditions.
> 
> Here's series of small improvements we have originally used on top
> of v1.7.0 (now rebased to current master).
> 
> Tero Aho (3):
>   pcap: utilize underlying real interface properties
>   pcap: add support for jumbo frames
>   pcap: add byte and error counters into statistics
> 
>  lib/librte_pmd_pcap/rte_eth_pcap.c | 176 +++++++++++++++++++++++++++++++------
>  1 file changed, 151 insertions(+), 25 deletions(-)

Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements

[Mcnamara, John]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, April 28, 2015 11:10 AM
> To: Nicolás Pernas Maradei; Mcnamara, John
> Cc: dev@dpdk.org; Tero Aho
> Subject: Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements
> 
> Please Nicolas, John,
> could we have a review of these patches?

Will do.

John.
-- 

Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tero Aho
> Sent: Friday, February 27, 2015 1:43 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 0/3] pcap pmd improvements
> 
> We have found it convenient to use pcap interfaces in early development
> when speed does not matter. However, pcap interfaces use fixed values for
> some properties like link status which makes it hard to simulate certain
> conditions.
> 
> Here's series of small improvements we have originally used on top of
> v1.7.0 (now rebased to current master).


Hi,

Thanks for the patches. They seem useful.

The patch files have converted tabs to whitespace so they don't apply cleanly without changing the leading whitespace back to tabs. Perhaps you can fix that in v2.

John

Re: [dpdk-dev] [PATCH 1/3] pcap: utilize underlying real interface properties

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tero Aho
> Sent: Friday, February 27, 2015 1:43 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/3] pcap: utilize underlying real interface
> properties
> 
> These changes set pcap interface mac address to the real underlying
> interface address instead of the default one. Also real interface link
> status, speed and duplex are reported when eth_link_update is called for
> the pcap interface.
> 
> Signed-off-by: Tero Aho <tero.aho@coriant.com>

> ...
> +               /* get link status, speed and duplex from the underlying interface */
> +
> +               strncpy(ifr.ifr_name, internals->if_name,
> sizeof(ifr.ifr_name)-1);
> +               ifr.ifr_name[sizeof(ifr.ifr_name)-1] = 0;
> +               if (!ioctl(internals->if_fd, SIOCGIFFLAGS, &ifr))
> +                       dev->data->dev_link.link_status = (ifr.ifr_flags
> + & IFF_UP) ? 1 : 0;
> +
> +               cmd.cmd = ETHTOOL_GSET;
> +               ifr.ifr_data = (void *)&cmd;
> +               if (!ioctl(internals->if_fd, SIOCETHTOOL, &ifr)) {
> +                       dev->data->dev_link.link_speed =
> ethtool_cmd_speed(&cmd);


Hi Tero,

I can see the benefit of setting the link status and speed etc. but this method seems a little fragile. What if there isn't an underlying interface, or all the interfaces are already bound to DPDK? Also I don't think the ethtool calls will work on FreeBSD as is.

So overall, I think this is a nack for this part of the patch. Perhaps a better approach would be to provide a function so that the calling application can set the parameters instead.

John

Re: [dpdk-dev] [PATCH 2/3] pcap: add support for jumbo frames

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tero Aho
> Sent: Friday, February 27, 2015 1:43 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/3] pcap: add support for jumbo frames
> 
> Extend eth_pcap_rx and eth_pcap_tx to support jumbo frames.
> On receive side read large packets into multiple mbufs and on the transmit
> side do the opposite.
> 


> +eth_pcap_tx_jumbo(pcap_t *pcap,
> +                 struct rte_mbuf *mbuf) {
> +       u_char data[ETHER_MAX_JUMBO_FRAME_LEN];
> +       uint16_t data_len = 0;
> +
> +       while (mbuf != NULL) {
> +               /* There is no writev style function in libpcap, */
> +               /* we unfortunately have to copy data to a buffer. */
> +               rte_memcpy(data + data_len, rte_pktmbuf_mtod(mbuf, void
> *),
> +                          mbuf->data_len);
> +               data_len += mbuf->data_len;
> +               mbuf = mbuf->next;
> +       }

Hi Tero,

Not sure if it is possible in practice to overflow data[ETHER_MAX_JUMBO_FRAME_LEN] but there should probably be a check.

John.
-- 

Re: [dpdk-dev] [PATCH 3/3] pcap: add byte and error counters into statistics

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tero Aho
> Sent: Friday, February 27, 2015 1:43 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 3/3] pcap: add byte and error counters into
> statistics
> 
> Added input/ouput byte counters into pcap interface statistics, also calls
> pcap_stats to add dropped packets into input errors.
> 
> Signed-off-by: Tero Aho <tero.aho@coriant.com>
> ---
>  lib/librte_pmd_pcap/rte_eth_pcap.c | 37 +++++++++++++++++++++++++++++----
> ----
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c
> b/lib/librte_pmd_pcap/rte_eth_pcap.c
> index 3f23f0a..1c472ac 100644
> --- a/lib/librte_pmd_pcap/rte_eth_pcap.c
> +++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
> @@ -71,6 +71,7 @@ struct pcap_rx_queue {
>         uint8_t in_port;
>         struct rte_mempool *mb_pool;
>         volatile unsigned long rx_pkts;
> +       volatile unsigned long rx_octs;


Hi Tero,

The equivalent rx/tx_octs members in rte_eth_stats are called ibytes/obytes so these variables should be called rx_bytes/tx_bytes for consistency.

Also, not really your issue, but do these members actually need to be volatile for pcap?

Otherwise looks okay.

John.
-- 

Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements

[Nicolas Pernas Maradei]

Hi,

I had a look to the patch set last night and it looks good to me apart 
from the few comments that I've already shared with Tero.

Nico.

On 28/04/15 13:09, Mcnamara, John wrote:
>> -----Original Message-----
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>> Sent: Tuesday, April 28, 2015 11:10 AM
>> To: Nicolás Pernas Maradei; Mcnamara, John
>> Cc: dev@dpdk.org; Tero Aho
>> Subject: Re: [dpdk-dev] [PATCH 0/3] pcap pmd improvements
>>
>> Please Nicolas, John,
>> could we have a review of these patches?
> Will do.
>
> John.

Re: [dpdk-dev] [dpdk-dev,2/3] pcap: add support for jumbo frames

[Maxim Uvarov]

Patch looks good for me. But in latest dpdk file path changed, but it 
applies with small reject.

Reviewed-by: Maxim Uvarov <maxim.uvarov@linaro.org>

Maxim.

On 02/27/15 16:42, Tero Aho wrote:
> Extend eth_pcap_rx and eth_pcap_tx to support jumbo frames.
> On receive side read large packets into multiple mbufs and
> on the transmit side do the opposite.
>
> Signed-off-by: Tero Aho <tero.aho@coriant.com>
>
> ---
> lib/librte_pmd_pcap/rte_eth_pcap.c | 88 ++++++++++++++++++++++++++++++++------
>   1 file changed, 75 insertions(+), 13 deletions(-)
>
> --
> 1.9.1
>
>
> ============================================================
> The information contained in this message may be privileged
> and confidential and protected from disclosure. If the reader
> of this message is not the intended recipient, or an employee
> or agent responsible for delivering this message to the
> intended recipient, you are hereby notified that any reproduction,
> dissemination or distribution of this communication is strictly
> prohibited. If you have received this communication in error,
> please notify us immediately by replying to the message and
> deleting it from your computer. Thank you. Coriant-Tellabs
> ============================================================
>
> diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c b/lib/librte_pmd_pcap/rte_eth_pcap.c
> index 289af28..3f23f0a 100644
> --- a/lib/librte_pmd_pcap/rte_eth_pcap.c
> +++ b/lib/librte_pmd_pcap/rte_eth_pcap.c
> @@ -52,7 +52,7 @@
>   #include <pcap.h>
>
>   #define RTE_ETH_PCAP_SNAPSHOT_LEN 65535
> -#define RTE_ETH_PCAP_SNAPLEN 4096
> +#define RTE_ETH_PCAP_SNAPLEN ETHER_MAX_JUMBO_FRAME_LEN
>   #define RTE_ETH_PCAP_PROMISC 1
>   #define RTE_ETH_PCAP_TIMEOUT -1
>   #define ETH_PCAP_RX_PCAP_ARG  "rx_pcap"
> @@ -132,6 +132,43 @@ static struct rte_eth_link pmd_link = {
>                  .link_status = 0
>   };
>
> +/*
> + * Helper function to read mbuf chain.
> + */
> +static int
> +eth_pcap_rx_jumbo(struct rte_mempool *mb_pool,
> +                 struct rte_mbuf *mbuf,
> +                 const u_char *data,
> +                 uint16_t data_len)
> +{
> +       struct rte_mbuf *m = mbuf;
> +       /* copy first segment */
> +       uint16_t len = rte_pktmbuf_tailroom(mbuf);
> +       rte_memcpy(rte_pktmbuf_append(mbuf, len), data, len);
> +       data_len -= len;
> +       data += len;
> +
> +       while (data_len > 0) {
> +               /* allocate next mbuf and point to that */
> +               m->next = rte_pktmbuf_alloc(mb_pool);
> +               if (unlikely(m->next == NULL))
> +                       return -1;
> +               m = m->next;
> +
> +               /* no headroom needed in chained mbufs */
> +               rte_pktmbuf_prepend(m, rte_pktmbuf_headroom(m));
> +               m->data_len = m->pkt_len = 0;
> +
> +               /* copy next segment */
> +               len = RTE_MIN(rte_pktmbuf_tailroom(m), data_len);
> +               rte_memcpy(rte_pktmbuf_append(m, len), data, len);
> +
> +               mbuf->nb_segs++;
> +               data_len -= len;
> +               data += len;
> +       }
> +       return mbuf->nb_segs;
> +}
>
>   static uint16_t
>   eth_pcap_rx(void *queue,
> @@ -173,17 +210,18 @@ eth_pcap_rx(void *queue,
>                          rte_memcpy(rte_pktmbuf_mtod(mbuf, void *), packet,
>                                          header.len);
>                          mbuf->data_len = (uint16_t)header.len;
> -                       mbuf->pkt_len = mbuf->data_len;
> -                       mbuf->port = pcap_q->in_port;
> -                       bufs[num_rx] = mbuf;
> -                       num_rx++;
>                  } else {
> -                       /* pcap packet will not fit in the mbuf, so drop packet */
> -                       RTE_LOG(ERR, PMD,
> -                                       "PCAP packet %d bytes will not fit in mbuf (%d bytes)\n",
> -                                       header.len, buf_size);
> -                       rte_pktmbuf_free(mbuf);
> +                       /* pcap packet will not fit in a single mbuf */
> +                       if (unlikely(eth_pcap_rx_jumbo(pcap_q->mb_pool, mbuf,
> +                                       packet, header.len) == -1)) {
> +                               rte_pktmbuf_free(mbuf);
> +                               break;
> +                       }
>                  }
> +               mbuf->pkt_len = (uint16_t)header.len;
> +               mbuf->port = pcap_q->in_port;
> +               bufs[num_rx] = mbuf;
> +               num_rx++;
>          }
>          pcap_q->rx_pkts += num_rx;
>          return num_rx;
> @@ -241,6 +279,27 @@ eth_pcap_tx_dumper(void *queue,
>   }
>
>   /*
> + * Helper function to write mbuf chain.
> + */
> +static uint16_t
> +eth_pcap_tx_jumbo(pcap_t *pcap,
> +                 struct rte_mbuf *mbuf)
> +{
> +       u_char data[ETHER_MAX_JUMBO_FRAME_LEN];
> +       uint16_t data_len = 0;
> +
> +       while (mbuf != NULL) {
> +               /* There is no writev style function in libpcap, */
> +               /* we unfortunately have to copy data to a buffer. */
> +               rte_memcpy(data + data_len, rte_pktmbuf_mtod(mbuf, void *),
> +                          mbuf->data_len);
> +               data_len += mbuf->data_len;
> +               mbuf = mbuf->next;
> +       }
> +       return pcap_sendpacket(pcap, data, data_len);
> +}
> +
> +/*
>    * Callback to handle sending packets through a real NIC.
>    */
>   static uint16_t
> @@ -259,9 +318,12 @@ eth_pcap_tx(void *queue,
>
>          for (i = 0; i < nb_pkts; i++) {
>                  mbuf = bufs[i];
> -               ret = pcap_sendpacket(tx_queue->pcap,
> -                               rte_pktmbuf_mtod(mbuf, u_char *),
> -                               mbuf->data_len);
> +               if (likely(mbuf->nb_segs == 1))
> +                       ret = pcap_sendpacket(tx_queue->pcap,
> +                                             rte_pktmbuf_mtod(mbuf, u_char *),
> +                                             mbuf->data_len);
> +               else
> +                       ret = eth_pcap_tx_jumbo(tx_queue->pcap, mbuf);
>                  if (unlikely(ret != 0))
>                          break;
>                  num_tx++;

Re: [dpdk-dev] [dpdk-dev,2/3] pcap: add support for jumbo frames

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxim Uvarov
> Sent: Wednesday, June 10, 2015 10:08 AM
> To: Tero Aho; dev@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-dev,2/3] pcap: add support for jumbo frames
> 
> Patch looks good for me. But in latest dpdk file path changed, but it
> applies with small reject.

Hi,

Thanks for the feedback.

I'll rebase that part of the patchset and resubmit it.

I'll review the status of the other 2 patches in the patchset as well.

John.
--