[dpdk-dev] [PATCH v3] ixgbe: fix data access on big endian cpu

[Ananyev, Konstantin]

Hi,

> -Original Message-
> From: xuelin.shi at freescale.com [mailto:xuelin.shi at freescale.com]
> Sent: Tuesday, March 31, 2015 8:26 AM
> To: Ananyev, Konstantin
> Cc: dev at dpdk.org; thomas.monjalon at 6wind.com; Xuelin Shi
> Subject: [PATCH v3] ixgbe: fix data access on big endian cpu
> 
> From: Xuelin Shi 
> 
> enforce rules of the cpu and ixgbe exchange data.
> 1. cpu use data owned by ixgbe must use rte_le_to_cpu_xx(...)
> 2. cpu fill data to ixgbe must use rte_cpu_to_le_xx(...)
> 3. checking pci status with converted constant.
> 
> Signed-off-by: Xuelin Shi 
> ---
> change for v3:
>check pci status with converted constant to avoid performance penalty.
>remove tmp variable.
> 
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 89 
> ---
>  1 file changed, 56 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index 9da2c7e..6e508ec 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -129,7 +129,7 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
> 
>   /* check DD bit on threshold descriptor */
>   status = txq->tx_ring[txq->tx_next_dd].wb.status;
> - if (! (status & IXGBE_ADVTXD_STAT_DD))
> + if (!(status & rte_cpu_to_le_32(IXGBE_ADVTXD_STAT_DD)))
>   return 0;
> 
>   /*
> @@ -174,11 +174,14 @@ tx4(volatile union ixgbe_adv_tx_desc *txdp, struct 
> rte_mbuf **pkts)
>   pkt_len = (*pkts)->data_len;
> 
>   /* write data to descriptor */
> - txdp->read.buffer_addr = buf_dma_addr;
> + txdp->read.buffer_addr = rte_cpu_to_le_64(buf_dma_addr);
> +
>   txdp->read.cmd_type_len =
> - ((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
> + rte_cpu_to_le_32((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
> +
>   txdp->read.olinfo_status =
> - (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
> + rte_cpu_to_le_32(pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
> +
>   rte_prefetch0(&(*pkts)->pool);
>   }
>  }
> @@ -194,11 +197,14 @@ tx1(volatile union ixgbe_adv_tx_desc *txdp, struct 
> rte_mbuf **pkts)
>   pkt_len = (*pkts)->data_len;
> 
>   /* write data to descriptor */
> - txdp->read.buffer_addr = buf_dma_addr;
> + txdp->read.buffer_addr = rte_cpu_to_le_64(buf_dma_addr);
> +
>   txdp->read.cmd_type_len =
> - ((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
> + rte_cpu_to_le_32((uint32_t)DCMD_DTYP_FLAGS | pkt_len);
> +
>   txdp->read.olinfo_status =
> - (pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
> + rte_cpu_to_le_32(pkt_len << IXGBE_ADVTXD_PAYLEN_SHIFT);
> +
>   rte_prefetch0(&(*pkts)->pool);
>  }
> 
> @@ -285,7 +291,7 @@ tx_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>* a divisor of the ring size
>*/
>   tx_r[txq->tx_next_rs].read.cmd_type_len |=
> - rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS);
> + rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS);
>   txq->tx_next_rs = (uint16_t)(txq->tx_rs_thresh - 1);
> 
>   txq->tx_tail = 0;
> @@ -304,7 +310,7 @@ tx_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>*/
>   if (txq->tx_tail > txq->tx_next_rs) {
>   tx_r[txq->tx_next_rs].read.cmd_type_len |=
> - rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS);
> + rte_cpu_to_le_32(IXGBE_ADVTXD_DCMD_RS);
>   txq->tx_next_rs = (uint16_t)(txq->tx_next_rs +
>   txq->tx_rs_thresh);
>   if (txq->tx_next_rs >= txq->nb_tx_desc)
> @@ -505,6 +511,7 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)
>   uint16_t nb_tx_desc = txq->nb_tx_desc;
>   uint16_t desc_to_clean_to;
>   uint16_t nb_tx_to_clean;
> + uint32_t stat;
> 
>   /* Determine the last descriptor needing to be cleaned */
>   desc_to_clean_to = (uint16_t)(last_desc_cleaned + txq->tx_rs_thresh);
> @@ -513,7 +520,9 @@ ixgbe_xmit_cleanup(struct ixgbe_tx_queue *txq)
> 
>   /* Check to make sure the last descriptor to clean is done */
>   desc_to_clean_to = sw_ring[desc_to_clean_to].last_id;
> - if (! (txr[desc_to_clean_to].wb.status & IXGBE_TXD_STAT_DD))
> +
> + stat = txr[desc_to_clean_to].wb.status;
> + if (!(stat & rte_cpu_to_le_32(IXGBE_TXD_STAT_DD)))
>   {
>   PMD_TX_FREE_LOG(DEBUG,
>   "TX descriptor %4u is not done"
> @@ -801,12 +810,14 @@ ixgbe_xmit_pkts(void *tx_queue, struct rte_mbuf 
> **tx_pkts,
>*/
>   slen = m_seg->data_len;
>   buf_dma_addr = RTE_MBUF_DATA_DMA_ADDR(m_seg);
> +
>   txd->read.buffer_addr =
> - rt

[dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts.

[Ananyev, Konstantin]

Hi,

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of WangDong
> Sent: Saturday, April 11, 2015 4:34 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] ixgbe:Add write memory barrier for recv pkts.
> 
> Like transmit packets, before update receive descriptor's tail pointer, 
> rte_wmb() should be added after writing recv descriptor.
> 
> Signed-off-by: Dong Wang 
> ---
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index 9da2c7e..d504688 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -1338,6 +1338,9 @@ ixgbe_recv_pkts(void *rx_queue, struct rte_mbuf 
> **rx_pkts,
>*/
>   rx_pkts[nb_rx++] = rxm;
>   }
> +
> + rte_wmb();
> +

Why do you think it is necessary?
I can't see any good reason to put wmb() here.
I would understand if, at least you'll try to insert it just before updating 
RDT:
 rx_id = (uint16_t) ((rx_id == 0) ?
 (rxq->nb_rx_desc - 1) : (rx_id - 1));
+ rte_wmb();
IXGBE_PCI_REG_WRITE(rxq->rdt_reg_addr, rx_id);

That is not needed IA with current implementation, but would make sense for 
machines with relaxed memory ordering.
Though right now DPDK IXGBE PMD is supported only on IA,  anyway.
Same for ixgbe_recv_scattered_pkts().

Konstantin


>   rxq->rx_tail = rx_id;
> 
>   /*
> @@ -1595,6 +1598,8 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct 
> rte_mbuf **rx_pkts,
>   first_seg = NULL;
>   }
> 
> + rte_wmb();
> +
>   /*
>* Record index of the next RX descriptor to probe.
>*/
> --
> 1.9.1

[dpdk-dev] [PATCH v5] Restore support for virtio on FreeBSD

[Ananyev, Konstantin]

Hi,

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Raz Amir
> Sent: Tuesday, April 14, 2015 5:23 PM
> To: dev at dpdk.org
> Cc: Raz Amir
> Subject: [dpdk-dev] [PATCH v5] Restore support for virtio on FreeBSD
> 
> Fixes: 8a312224bcde ("eal/bsd: fix fd leak")
> 
> Closing /dev/io fd causes SIGBUS in inb/outb instructions
> as the process loses the IOPL privileges once the fd is closed:
> (gdb) bt
> 0  0x00492f2c in outb (port=49170, data=0 '\000')
> at /usr/include/machine/cpufunc.h:244
> 1  0x00492f7a in outb_p (data=0 '\000', port=49170)
> at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.h:211
> 2  0x0049328d in vtpci_set_status (hw=0x80331f380, status=0 '\000')
> at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.c:130
> 3  0x004931fe in vtpci_reset (hw=0x80331f380)
> at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.c:108
> 4  0x004a175e in eth_virtio_dev_init (eth_dev=0x831b80 
> )
> at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_ethdev.c:1150
> 5  0x00462c09 in rte_eth_dev_init (pci_drv=0x79d1a0 ,
> pci_dev=0x802417560) at /dpdk/dpdk-2.0.0/lib/librte_ether/rte_ethdev.c:326
> 6  0x0046f03f in rte_eal_pci_probe_one_driver (dr=0x79d1a0 
> ,
> dev=0x802417560) at 
> /dpdk/dpdk-2.0.0/lib/librte_eal/bsdapp/eal/eal_pci.c:487
> 7  0x00475b06 in pci_probe_all_drivers (dev=0x802417560)
> at /dpdk/dpdk-2.0.0/lib/librte_eal/common/eal_common_pci.c:116
> 8  0x00475bb9 in rte_eal_pci_probe ()
> at /dpdk/dpdk-2.0.0/lib/librte_eal/common/eal_common_pci.c:246
> 9  0x0046cd63 in rte_eal_init (argc=5, argv=0x7fffeaf0)
> at /dpdk/dpdk-2.0.0/lib/librte_eal/bsdapp/eal/eal.c:554
> 10 0x00404544 in main ()
> 
> Signed-off-by: Raz Amir 
> ---
>  lib/librte_eal/bsdapp/eal/eal.c | 19 ++-
>  lib/librte_eal/common/include/rte_eal.h | 10 ++
>  lib/librte_eal/linuxapp/eal/eal.c   |  5 +
>  lib/librte_pmd_virtio/virtio_ethdev.c   |  9 +
>  4 files changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
> index 871d5f4..687dd83 100644
> --- a/lib/librte_eal/bsdapp/eal/eal.c
> +++ b/lib/librte_eal/bsdapp/eal/eal.c
> @@ -112,6 +112,9 @@ struct internal_config internal_config;
>  /* used by rte_rdtsc() */
>  int rte_cycles_vmware_tsc_map;
> 
> +/* fd to keep open for iopl */
> +static int iopl_fd = -1;
> +
>  /* Return a pointer to the configuration structure */
>  struct rte_config *
>  rte_eal_get_configuration(void)
> @@ -421,15 +424,21 @@ int rte_eal_has_hugepages(void)
>  int
>  rte_eal_iopl_init(void)
>  {
> - int fd;
> -
> - fd = open("/dev/io", O_RDWR);
> - if (fd < 0)
> + iopl_fd = open("/dev/io", O_RDWR);
> + if (iopl_fd < 0)
>   return -1;
> - close(fd);
> + /* keep fd open for iopl */
>   return 0;
>  }
> 
> +void
> +rte_eal_iopl_uninit(void)
> +{
> + if (iopl_fd != -1)
> + close(iopl_fd);
> + iopl_fd = -1;
> +}

Did I get it right: that function would be invoked for at dev_detach()?
And after we invoked it, we still we can have other multiple virtio devices 
attached and active?
If so, then I suppose you'll hit the same problem again.
Konstantin  

> +
>  /* Launch threads, called at application init(). */
>  int
>  rte_eal_init(int argc, char **argv)
> diff --git a/lib/librte_eal/common/include/rte_eal.h 
> b/lib/librte_eal/common/include/rte_eal.h
> index 1385a73..9151e08 100644
> --- a/lib/librte_eal/common/include/rte_eal.h
> +++ b/lib/librte_eal/common/include/rte_eal.h
> @@ -127,6 +127,16 @@ enum rte_proc_type_t rte_eal_process_type(void);
>  int rte_eal_iopl_init(void);
> 
>  /**
> + * Release iopl priviledge - currently relevant only for FreeBSD.
> + *
> + * This function should be called by pmds which need access to ioports.
> +
> + * @return
> + *   void
> + */
> +void rte_eal_iopl_uninit(void);
> +
> +/**
>   * Initialize the Environment Abstraction Layer (EAL).
>   *
>   * This function is to be executed on the MASTER lcore only, as soon
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
> b/lib/librte_eal/linuxapp/eal/eal.c
> index bd770cf..687cebf 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -695,6 +695,11 @@ rte_eal_iopl_init(void)
>  #endif
>  }
> 
> +void
> +rte_eal_iopl_uninit(void)
> +{
> +}
> +
>  /* Launch threads, called at application init(). */
>  int
>  rte_eal_init(int argc, char **argv)
> diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c 
> b/lib/librte_pmd_virtio/virtio_ethdev.c
> index 7b83d9b..5be5c27 100644
> --- a/lib/librte_pmd_virtio/virtio_ethdev.c
> +++ b/lib/librte_pmd_virtio/virtio_ethdev.c
> @@ -1265,6 +1265,14 @@ rte_virtio_pmd_init(const char *name __rte_unused,
>   return 0;
>  }
> 
> +static int
> +rte_virtio_pmd_uninit(const char *name)
> +{
> + (void)name;
> +

[dpdk-dev] [PATCH] Clean up rte_memcpy.h file

[Stephen Hemminger]

On Tue, 14 Apr 2015 14:31:53 -0700
Ravi Kerur  wrote:

> +
> + for (i = 0; i < 2; i++)
> + rte_mov32(dst + i * 32, src + i * 32);
>  }
Unless you force compiler to unroll the loop, it will be slower.

[dpdk-dev] [PATCH v5] Restore support for virtio on FreeBSD

[Raz Amir]

Fixes: 8a312224bcde ("eal/bsd: fix fd leak")

Closing /dev/io fd causes SIGBUS in inb/outb instructions
as the process loses the IOPL privileges once the fd is closed:
(gdb) bt
0  0x00492f2c in outb (port=49170, data=0 '\000')
at /usr/include/machine/cpufunc.h:244
1  0x00492f7a in outb_p (data=0 '\000', port=49170)
at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.h:211
2  0x0049328d in vtpci_set_status (hw=0x80331f380, status=0 '\000')
at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.c:130
3  0x004931fe in vtpci_reset (hw=0x80331f380)
at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_pci.c:108
4  0x004a175e in eth_virtio_dev_init (eth_dev=0x831b80 
)
at /dpdk/dpdk-2.0.0/lib/librte_pmd_virtio/virtio_ethdev.c:1150
5  0x00462c09 in rte_eth_dev_init (pci_drv=0x79d1a0 ,
pci_dev=0x802417560) at /dpdk/dpdk-2.0.0/lib/librte_ether/rte_ethdev.c:326
6  0x0046f03f in rte_eal_pci_probe_one_driver (dr=0x79d1a0 
,
dev=0x802417560) at /dpdk/dpdk-2.0.0/lib/librte_eal/bsdapp/eal/eal_pci.c:487
7  0x00475b06 in pci_probe_all_drivers (dev=0x802417560)
at /dpdk/dpdk-2.0.0/lib/librte_eal/common/eal_common_pci.c:116
8  0x00475bb9 in rte_eal_pci_probe ()
at /dpdk/dpdk-2.0.0/lib/librte_eal/common/eal_common_pci.c:246
9  0x0046cd63 in rte_eal_init (argc=5, argv=0x7fffeaf0)
at /dpdk/dpdk-2.0.0/lib/librte_eal/bsdapp/eal/eal.c:554
10 0x00404544 in main ()

Signed-off-by: Raz Amir 
---
 lib/librte_eal/bsdapp/eal/eal.c | 19 ++-
 lib/librte_eal/common/include/rte_eal.h | 10 ++
 lib/librte_eal/linuxapp/eal/eal.c   |  5 +
 lib/librte_pmd_virtio/virtio_ethdev.c   |  9 +
 4 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 871d5f4..687dd83 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -112,6 +112,9 @@ struct internal_config internal_config;
 /* used by rte_rdtsc() */
 int rte_cycles_vmware_tsc_map;

+/* fd to keep open for iopl */
+static int iopl_fd = -1;
+
 /* Return a pointer to the configuration structure */
 struct rte_config *
 rte_eal_get_configuration(void)
@@ -421,15 +424,21 @@ int rte_eal_has_hugepages(void)
 int
 rte_eal_iopl_init(void)
 {
-   int fd;
-
-   fd = open("/dev/io", O_RDWR);
-   if (fd < 0)
+   iopl_fd = open("/dev/io", O_RDWR);
+   if (iopl_fd < 0)
return -1;
-   close(fd);
+   /* keep fd open for iopl */
return 0;
 }

+void
+rte_eal_iopl_uninit(void)
+{
+   if (iopl_fd != -1)
+   close(iopl_fd);
+   iopl_fd = -1;
+}
+
 /* Launch threads, called at application init(). */
 int
 rte_eal_init(int argc, char **argv)
diff --git a/lib/librte_eal/common/include/rte_eal.h 
b/lib/librte_eal/common/include/rte_eal.h
index 1385a73..9151e08 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -127,6 +127,16 @@ enum rte_proc_type_t rte_eal_process_type(void);
 int rte_eal_iopl_init(void);

 /**
+ * Release iopl priviledge - currently relevant only for FreeBSD.
+ *
+ * This function should be called by pmds which need access to ioports.
+
+ * @return
+ *   void
+ */
+void rte_eal_iopl_uninit(void);
+
+/**
  * Initialize the Environment Abstraction Layer (EAL).
  *
  * This function is to be executed on the MASTER lcore only, as soon
diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
b/lib/librte_eal/linuxapp/eal/eal.c
index bd770cf..687cebf 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -695,6 +695,11 @@ rte_eal_iopl_init(void)
 #endif
 }

+void
+rte_eal_iopl_uninit(void)
+{
+}
+
 /* Launch threads, called at application init(). */
 int
 rte_eal_init(int argc, char **argv)
diff --git a/lib/librte_pmd_virtio/virtio_ethdev.c 
b/lib/librte_pmd_virtio/virtio_ethdev.c
index 7b83d9b..5be5c27 100644
--- a/lib/librte_pmd_virtio/virtio_ethdev.c
+++ b/lib/librte_pmd_virtio/virtio_ethdev.c
@@ -1265,6 +1265,14 @@ rte_virtio_pmd_init(const char *name __rte_unused,
return 0;
 }

+static int
+rte_virtio_pmd_uninit(const char *name)
+{
+   (void)name;
+   rte_eal_iopl_uninit();
+   return 0;
+}
+
 /*
  * Only 1 queue is supported, no queue release related operation
  */
@@ -1499,6 +1507,7 @@ __rte_unused uint8_t is_rx)
 static struct rte_driver rte_virtio_driver = {
.type = PMD_PDEV,
.init = rte_virtio_pmd_init,
+   .uninit = rte_virtio_pmd_uninit,
 };

 PMD_REGISTER_DRIVER(rte_virtio_driver);
-- 
2.1.2

[dpdk-dev] [PATCH v3] Restore support for virtio on FreeBSD

[Raz Amir]

Thomas, I will add more information to the commit message, but regarding
your feedback on the iopl comment, it is called also iopl on FreeBSD.
See this link to FreeBSD source code, for the io driver code - the flag name
is PSL_IOPL:
https://github.com/freebsd/freebsd/blob/master/sys/i386/i386/io.c#L38

Ouyang, I will implement your suggestion in the next patch version I submit.


-Original Message-
From: Ouyang, Changchun [mailto:changchun.ouy...@intel.com] 
Sent: 14 April 2015 05:33
To: Thomas Monjalon; Raz Amir
Cc: dev at dpdk.org; Ouyang, Changchun
Subject: RE: [dpdk-dev] [PATCH v3] Restore support for virtio on FreeBSD

Hi 

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, April 13, 2015 8:55 PM
> To: Raz Amir
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] Restore support for virtio on 
> FreeBSD
> 
> Please provide more information in the commit message.
> We need to know what was the problem (crash) in the git history.
> Then when doing git blame, we'll have the full explanation.
> 
> 2015-04-13 15:19, Raz Amir:
> > Fixes: 8a312224bcde ("eal/bsd: fix fd leak")
> >
> > Signed-off-by: Raz Amir 
> > ---
> >  lib/librte_eal/bsdapp/eal/eal.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/eal.c 
> > b/lib/librte_eal/bsdapp/eal/eal.c index 871d5f4..e20f915 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal.c
> > @@ -426,7 +426,7 @@ rte_eal_iopl_init(void)
> > fd = open("/dev/io", O_RDWR);
> > if (fd < 0)
> > return -1;
> > -   close(fd);
> > +   /* keep fd open for iopl */

Copy and paste my comment into this new patch:
Would you pls think about this solution?
Declare a static var to keep the fd which is opened for freebsd; Then define
a deinit function for virtio device, Inside the deinit function, close the
fd which was opened in init stage.
Done.

thanks
Changchun

[dpdk-dev] How to check memory leak with dpdk application

[Linhaifeng]

On 2015/4/14 4:25, Marc Sune wrote:
> 
> 
> On 10/04/15 07:53, Linhaifeng wrote:
>> Hi, all
>>
>> I'am trying to use valgrind to check memory leak with my dpdk application 
>> but dpdk always failed to mmap hugepages.
>>
>> Without valgrind it works well.How to run dpdk applications with valgrind?Is 
>> there any other way to check memory leak
>> with dpdk applications?
>>
> 
> Yes it can be used, just that 3.10 has issues with hugepages. Check this out:
> 
> http://article.gmane.org/gmane.comp.networking.dpdk.devel/8058/match=valgrind+hugepages
> 
> Marc
> 
> 

Hi?Marc

Thank you very much!

[dpdk-dev] tools brainstorming

[Wiles, Keith]

On 4/14/15, 11:19 AM, "Wiles, Keith"  wrote:

>
>
>On 4/14/15, 10:24 AM, "Thomas Monjalon"  wrote:
>
>>2015-04-14 15:52, Bruce Richardson:
>>> On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
>>> > When a consensus is done, it must be added with a patch with custom
>>> > checkpatch addition.
>>> > 
>>> My personal feeling is that we should try and keep checkpatch
>>>modifications to a
>>> minimum. Right now, we can use checkpatch as-is from kernel.org, right?
>>
>>Yes that's something we have to discuss.
>>It should be preferred to avoid "forking" checkpatch.
>>
>>At the moment, I'm using this configuration:
>>
>>  options="$options --max-line-length=100"
>>  options="$options --show-types"
>>  options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
>>  VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
>>  SPLIT_STRING,LINE_SPACING,NEW_TYPEDEFS,COMPLEX_MACRO"
>>
>>  linux/scripts/checkpatch.pl $options
>>
>>I would like to submit a script to run checkpatch with DPDK configuration
>>when the coding rules are clear.
>>
>>However, I've already seen some options which are not enough configurable
>>(don't remember which one). For such corner case, I would see 3 solutions
>>(from the most to the least desired):
>>  - submit a patch to allow more configuration to kernel.org
>>  - give up automatic handling of corner cases
>>  - maintain a fork in scripts/ directory
>Here is the next solution
>   - Stop using checkpatch and use a real tool for formatting code instead.
>If someone uses a tool before commit, then create the patch which does not
>require checkpatch.
>Most of these tools can define an output file or they leave behind the
>original file as a backup or we can see if they have a non-modify mode and
>just points out the problems. As in astyle '--dry-run' can be used, plus
>it saves the original file as X.orig or you can change the .orig to
>your own value.
>>

Using uncrustify with following config file seems to be very close to what
we have today and removes trailing white spaces. I changed the
indent_with_tabs to 2 instead of 1. I am sure we could even get closer.
The uncrustify creates the updated file into X.uncrustify which does
not effect the original file. Then you can use meld or some other tool to
view the changes.

http://uncrustify.sourceforge.net/default.cfg


The source is here http://uncrustify.sourceforge.net/ pretty simple
install on my Ubuntu 14.04 machine.

I also installed the amd64 image of UniversalindentGUI. I tried the
?Ubuntu Software Center? version, but it required a bit more effort then
just installing. It appeared the code needed a specific version of a
library I did not track down.

http://uncrustify.sourceforge.net/


It looks like using uncrustify and a config file gives use a tool to
verify the code is formatted correctly before commit and patch create.

Have a look and see what you  think.

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 18:28, Thomas Monjalon wrote:
> 2015-04-14 18:21, Vlad Zolotarov:
>> On 04/14/15 18:13, Thomas Monjalon wrote:
>>> 2015-04-14 17:59, Vlad Zolotarov:
 On 04/14/15 17:17, Thomas Monjalon wrote:
> 2015-04-14 16:38, Vlad Zolotarov:
>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
 On 04/14/15 12:31, Thomas Monjalon wrote:
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
 Hmmm... Unless I miss something this and one above would zero only a
 single field - "max_rx_queues"; and would leave the rest uninitialized.
 The original code intend to zero the whole struct. The alternative to
 the original lines could be usage of memset().
>>> As I understand, in that case compiler had to set all non-explicitly 
>>> initialised members to 0.
>>> So I think we are ok here.
>> Yeah, I guess it does zero-initializes the rest
>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>> don't understand how the above change fixes the error if it complains
>> about the dev_info.driver_name?
> As only 1 field is required, I chose the one which should not be removed
> from this structure in the future.
>
>> What I'm trying to say - the proposed fix is completely unclear and
>> confusing. Think of somebody reading this line in a month from today -
>> he wouldn't get a clue why is it there, why to explicitly set
>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>> to add such artifacts to the code instead of just zeroing the struct
>> with a memset() and putting a good clear comment above it explaining why
>> we use a memset() and not and initializer?
> We can make it longer yes.
> I think you agree we should avoid extra lines if not needed.
> In this case, when reading "= { .field = 0 }", it seems clear our goal
> is to zero the structure (it is to me).
 I'm sorry but it's not clear to me at all since the common C practice
 for zeroing the struct would be

 struct st a = {0};

 Like in the lines u are changing. The lines as above are clearly should
 not be commented and are absolutely clear.
 The lines u are adding on the other hand are absolutely unclear and
 confusing outside the gcc bug context. Therefore it should be clearly
 stated so in a form of comment. Otherwise somebody (like myself) may see
 this and immediately fix it back (as it should be).

> I thought it is a basic C practice.
 I doubt that. ;) Explained above.

> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> not going to comment each occurence of this coding style.
> But it must be explained in the coding style document. Agree?
 OMG! This is awful! I think everybody agrees that this is a workaround
 and has nothing to do with a codding style (it's an opposite to a style
 actually). I don't know where this should be explained, frankly.
>>> Once we assert we want to support this buggy compiler, the workarounds
>>> are automatically parts of the coding style.
>> It'd rather not... ;)
>>
>>> I don't know how to deal differently with this constraint.
>> Add -Wno-missing-braces compilation option for compiler versions below
>> 4.7. U (and me and I guess most other developers) compile DPDK code with
>> a newer compiler thus the code would be properly inspected with these
>> compilers and we may afford to be less restrictive with compilation
>> warnings with legacy compiler versions...
> You're right.
> I will test it and submit a v2.
> Then I could use the above grep command to replace other occurences of this
> workaround.

U read my mind!.. ;)

>
 Getting back to the issue - I'm a bit surprised since I use this kind of
 initializer ({0}) in a C code for quite a long time - long before 2012.
 I'd like to understand what is a problem with this specific gcc version.
 This seems to trivial. I'm surprised CentOS has a gcc version with this
 kind of bugs.
>>> Each day brings its surprise :)
>

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 18:13, Thomas Monjalon wrote:
> 2015-04-14 17:59, Vlad Zolotarov:
>> On 04/14/15 17:17, Thomas Monjalon wrote:
>>> 2015-04-14 16:38, Vlad Zolotarov:
 On 04/14/15 16:06, Ananyev, Konstantin wrote:
> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>> -   struct rte_eth_dev_info dev_info = { 0 };
>>> +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>> Hmmm... Unless I miss something this and one above would zero only a
>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>> The original code intend to zero the whole struct. The alternative to
>> the original lines could be usage of memset().
> As I understand, in that case compiler had to set all non-explicitly 
> initialised members to 0.
> So I think we are ok here.
 Yeah, I guess it does zero-initializes the rest
 (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
 don't understand how the above change fixes the error if it complains
 about the dev_info.driver_name?
>>> As only 1 field is required, I chose the one which should not be removed
>>> from this structure in the future.
>>>
 What I'm trying to say - the proposed fix is completely unclear and
 confusing. Think of somebody reading this line in a month from today -
 he wouldn't get a clue why is it there, why to explicitly set
 max_rx_queues to zero and leave the rest be zeroed automatically... Why
 to add such artifacts to the code instead of just zeroing the struct
 with a memset() and putting a good clear comment above it explaining why
 we use a memset() and not and initializer?
>>> We can make it longer yes.
>>> I think you agree we should avoid extra lines if not needed.
>>> In this case, when reading "= { .field = 0 }", it seems clear our goal
>>> is to zero the structure (it is to me).
>> I'm sorry but it's not clear to me at all since the common C practice
>> for zeroing the struct would be
>>
>> struct st a = {0};
>>
>> Like in the lines u are changing. The lines as above are clearly should
>> not be commented and are absolutely clear.
>> The lines u are adding on the other hand are absolutely unclear and
>> confusing outside the gcc bug context. Therefore it should be clearly
>> stated so in a form of comment. Otherwise somebody (like myself) may see
>> this and immediately fix it back (as it should be).
>>
>>> I thought it is a basic C practice.
>> I doubt that. ;) Explained above.
>>
>>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
>>> not going to comment each occurence of this coding style.
>>> But it must be explained in the coding style document. Agree?
>> OMG! This is awful! I think everybody agrees that this is a workaround
>> and has nothing to do with a codding style (it's an opposite to a style
>> actually). I don't know where this should be explained, frankly.
> Once we assert we want to support this buggy compiler, the workarounds
> are automatically parts of the coding style.

It'd rather not... ;)

> I don't know how to deal differently with this constraint.

Add -Wno-missing-braces compilation option for compiler versions below 
4.7. U (and me and I guess most other developers) compile DPDK code with 
a newer compiler thus the code would be properly inspected with these 
compilers and we may afford to be less restrictive with compilation 
warnings with legacy compiler versions...

>
>> Getting back to the issue - I'm a bit surprised since I use this kind of
>> initializer ({0}) in a C code for quite a long time - long before 2012.
>> I'd like to understand what is a problem with this specific gcc version.
>> This seems to trivial. I'm surprised CentOS has a gcc version with this
>> kind of bugs.
> Each day brings its surprise :)
>

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 17:53, Thomas Monjalon wrote:
> 2015-04-14 17:30, Vlad Zolotarov:
>> On 04/14/15 17:17, Thomas Monjalon wrote:
>>> 2015-04-14 16:38, Vlad Zolotarov:
 On 04/14/15 16:06, Ananyev, Konstantin wrote:
> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>> -   struct rte_eth_dev_info dev_info = { 0 };
>>> +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>> Hmmm... Unless I miss something this and one above would zero only a
>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>> The original code intend to zero the whole struct. The alternative to
>> the original lines could be usage of memset().
> As I understand, in that case compiler had to set all non-explicitly 
> initialised members to 0.
> So I think we are ok here.
 Yeah, I guess it does zero-initializes the rest
 (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
 don't understand how the above change fixes the error if it complains
 about the dev_info.driver_name?
>>> As only 1 field is required, I chose the one which should not be removed
>>> from this structure in the future.
>> I don't follow - where/why only one field is required? The function u
>> are patching uses "rx_offload_capa" field. Or u mean this gcc version
>> requires only one field? If so, could u, please, provide the errata u
>> are referring, since standard doesn't require any field and {0} is an
>> absolutely legal (and proper) initializer in this case...
> Honestly I don't really care what is "legal". The most important is to make
> it working with most C compilers with minimal overhead.

It's not just a "legal" - it's the most correct and robust way of 
initializing the struct that is promised to always work correctly. See 
here 
http://stackoverflow.com/questions/11152160/initializing-a-struct-to-0. 
What u hit here is (as appears) a well known Bug #53119 in gcc (see here 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119).

Have u considered adding the compilation options like 
-Wno-missing-braces that would silence this warning for say gcc versions 
below 4.7?

> You're right about the variable choice: rx_offload_capa is more appropriate.
> Are you OK for a v2 replacing max_rx_queues by rx_offload_capa?
>
 What I'm trying to say - the proposed fix is completely unclear and
 confusing. Think of somebody reading this line in a month from today -
 he wouldn't get a clue why is it there, why to explicitly set
 max_rx_queues to zero and leave the rest be zeroed automatically... Why
 to add such artifacts to the code instead of just zeroing the struct
 with a memset() and putting a good clear comment above it explaining why
 we use a memset() and not and initializer?
>>> We can make it longer yes.
>>> I think you agree we should avoid extra lines if not needed.
>>> In this case, when reading "= { .field = 0 }", it seems clear our goal
>>> is to zero the structure (it is to me).
>>> I thought it is a basic C practice.
>>>
>>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
>>> not going to comment each occurence of this coding style.
>>> But it must be explained in the coding style document. Agree?
>

[dpdk-dev] freeze with dpdk-2.0.0

[Olivier Deme]

Hi,

I am trying to use DPDK-2.0.0 on Fedora 20 running in a qemu virtual 
machine.
After loading the uio and igb_uio module and setting up hugepages, I try 
to run the helloworld demo application.
As soon as the helloworld displays the "hello from core 1, hello from 
core 0" messages, I loose all network connectivity to the VM.

If I try to run the helloworld application remotely the output freezes 
after the following:
# ./build/helloworld -c 3 -n 2
EAL: Detected lcore 0 as core 0 on socket 0
EAL: Detected lcore 1 as core 0 on socket 0
EAL: Detected lcore 2 as core 0 on socket 0
EAL: Detected lcore 3 as core 0 on socket 0
EAL: Support maximum 128 logical core(s) by configuration.
EAL: Detected 4 lcore(s)
EAL: VFIO modules not all loaded, skip VFIO support...
EAL: Setting up memory...
EAL: Ask a virtual area of 0x700 bytes
EAL: Virtual area found at 0x7f273be0 (size = 0x700)
EAL: Ask a virtual area of 0x20 bytes
EAL: Virtual area found at 0x7f273ba0 (size = 0x20)
EAL: Ask a virtual area of 0x20 bytes
EAL: Virtual area found at 0x7f273b60 (size = 0x20)
EAL: Ask a virtual area of 0xc0 bytes
EAL: Virtual area found at 0x7f273a80 (size = 0xc0)
EAL: Requesting 64 pages of size 2MB from socket 0
EAL: TSC frequency is ~1995193 KHz
EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using 
unreliable clock cycles !
EAL: Master lcore 0 is ready (tid=449e9900;cpuset=[0])
PMD: ENICPMD trace: rte_enic_pmd_init
EAL: lcore 1 is ready (tid=3a7ff700;cpuset=[1])


If I try to restart the network services, I get the error from syslog:
BUG: soft lockup - CPU#2 stuck for 22s!


The network devices exposed to the VM are 2 BCM5719 interfaces + 2 
82599ES interfaces:
02:00.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5719 
Gigabit Ethernet PCIe (rev 01)
02:00.1 Ethernet controller: Broadcom Corporation NetXtreme BCM5719 
Gigabit Ethernet PCIe (rev 01)
0d:00.0 Ethernet controller: Intel Corporation 82599ES 10-Gigabit 
SFI/SFP+ Network Connection (rev 01)
0d:00.1 Ethernet controller: Intel Corporation 82599ES 10-Gigabit 
SFI/SFP+ Network Connection (rev 01)


Within the VM, lspci shows:
00:03.0 Ethernet controller: Red Hat, Inc Virtio network device
00:04.0 Ethernet controller: Red Hat, Inc Virtio network device
00:05.0 Ethernet controller: Red Hat, Inc Virtio network device
00:06.0 Ethernet controller: Red Hat, Inc Virtio network device


After loading the kernel modules, I bind the interfaces to DPDK with:
/root/dpdk-2.0.0/tools/dpdk_nic_bind.py --bind=igb_uio 00:05.0
/root/dpdk-2.0.0/tools/dpdk_nic_bind.py --bind=igb_uio 00:06.0

Would you know what is causing this?

I haven't been able to try DPDK 1.8.0 because it doesn't compile on the 
latest Fedora Server 20.

Thanks for your help,
Olivier.


-- 
*Olivier Dem?*
*Druid Software Ltd.*
*Tel: +353 1 202 1831*
*Email: odeme at druidsoftware.com *
*URL: http://www.druidsoftware.com*
Druid Software: Monetising enterprise small cells solutions.


Druid_Footer_Logo

[dpdk-dev] UIO pci-generic support broke igb_uio

[Stephen Hemminger]

The addition of uio pci-generic broke use if igb_uio because
the wrong file descriptor is being used.

If I was a hard ass I would recommend uio pci-generic support
be reverted from 2.0 until/unless this fixed.

Failure mode is on startup:

EAL:  Error reading interrupts status for fd 0
PANIC in start_port()
rte_eth-dev_start: port=0 err=-5

The problem commit is:
commit 4a499c64959074ba6fa6a5a2b3a2a6aa10627fa1
Author: Danny Zhou 
Date:   Fri Feb 20 16:59:15 2015 +

eal/linux: enable uio_pci_generic support

Change the EAL PCI code so that it can work with both the
uio_pci_generic in-tree driver, as well as the igb_uio
DPDK-specific driver.

This involves changes to
1) Modify method of retrieving BAR resource mapping information
2) Mapping using resource files in /sys rather than /dev/uio*
2) Setup bus master bit in NIC's PCIe configuration space for
uio_pci_generic.

Signed-off-by: Danny Zhou 
Signed-off-by: Bruce Richardson 
Acked-by: Declan Doherty 

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 17:17, Thomas Monjalon wrote:
> 2015-04-14 16:38, Vlad Zolotarov:
>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
 On 04/14/15 12:31, Thomas Monjalon wrote:
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
 Hmmm... Unless I miss something this and one above would zero only a
 single field - "max_rx_queues"; and would leave the rest uninitialized.
 The original code intend to zero the whole struct. The alternative to
 the original lines could be usage of memset().
>>> As I understand, in that case compiler had to set all non-explicitly 
>>> initialised members to 0.
>>> So I think we are ok here.
>> Yeah, I guess it does zero-initializes the rest
>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>> don't understand how the above change fixes the error if it complains
>> about the dev_info.driver_name?
> As only 1 field is required, I chose the one which should not be removed
> from this structure in the future.
>
>> What I'm trying to say - the proposed fix is completely unclear and
>> confusing. Think of somebody reading this line in a month from today -
>> he wouldn't get a clue why is it there, why to explicitly set
>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>> to add such artifacts to the code instead of just zeroing the struct
>> with a memset() and putting a good clear comment above it explaining why
>> we use a memset() and not and initializer?
> We can make it longer yes.
> I think you agree we should avoid extra lines if not needed.
> In this case, when reading "= { .field = 0 }", it seems clear our goal
> is to zero the structure (it is to me).

I'm sorry but it's not clear to me at all since the common C practice 
for zeroing the struct would be

struct st a = {0};

Like in the lines u are changing. The lines as above are clearly should 
not be commented and are absolutely clear.
The lines u are adding on the other hand are absolutely unclear and 
confusing outside the gcc bug context. Therefore it should be clearly 
stated so in a form of comment. Otherwise somebody (like myself) may see 
this and immediately fix it back (as it should be).

> I thought it is a basic C practice.

I doubt that. ;) Explained above.

>
> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> not going to comment each occurence of this coding style.
> But it must be explained in the coding style document. Agree?

OMG! This is awful! I think everybody agrees that this is a workaround 
and has nothing to do with a codding style (it's an opposite to a style 
actually). I don't know where this should be explained, frankly.

Getting back to the issue - I'm a bit surprised since I use this kind of 
initializer ({0}) in a C code for quite a long time - long before 2012. 
I'd like to understand what is a problem with this specific gcc version. 
This seems to trivial. I'm surprised CentOS has a gcc version with this 
kind of bugs.

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 17:17, Thomas Monjalon wrote:
> 2015-04-14 16:38, Vlad Zolotarov:
>> On 04/14/15 16:06, Ananyev, Konstantin wrote:
>>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
 On 04/14/15 12:31, Thomas Monjalon wrote:
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
 Hmmm... Unless I miss something this and one above would zero only a
 single field - "max_rx_queues"; and would leave the rest uninitialized.
 The original code intend to zero the whole struct. The alternative to
 the original lines could be usage of memset().
>>> As I understand, in that case compiler had to set all non-explicitly 
>>> initialised members to 0.
>>> So I think we are ok here.
>> Yeah, I guess it does zero-initializes the rest
>> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>> don't understand how the above change fixes the error if it complains
>> about the dev_info.driver_name?
> As only 1 field is required, I chose the one which should not be removed
> from this structure in the future.

I don't follow - where/why only one field is required? The function u 
are patching uses "rx_offload_capa" field. Or u mean this gcc version 
requires only one field? If so, could u, please, provide the errata u 
are referring, since standard doesn't require any field and {0} is an 
absolutely legal (and proper) initializer in this case...

>
>> What I'm trying to say - the proposed fix is completely unclear and
>> confusing. Think of somebody reading this line in a month from today -
>> he wouldn't get a clue why is it there, why to explicitly set
>> max_rx_queues to zero and leave the rest be zeroed automatically... Why
>> to add such artifacts to the code instead of just zeroing the struct
>> with a memset() and putting a good clear comment above it explaining why
>> we use a memset() and not and initializer?
> We can make it longer yes.
> I think you agree we should avoid extra lines if not needed.
> In this case, when reading "= { .field = 0 }", it seems clear our goal
> is to zero the structure (it is to me).
> I thought it is a basic C practice.
>
> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> not going to comment each occurence of this coding style.
> But it must be explained in the coding style document. Agree?

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Thomas Monjalon]

2015-04-14 18:21, Vlad Zolotarov:
> 
> On 04/14/15 18:13, Thomas Monjalon wrote:
> > 2015-04-14 17:59, Vlad Zolotarov:
> >> On 04/14/15 17:17, Thomas Monjalon wrote:
> >>> 2015-04-14 16:38, Vlad Zolotarov:
>  On 04/14/15 16:06, Ananyev, Konstantin wrote:
> > From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
> >> On 04/14/15 12:31, Thomas Monjalon wrote:
> >>> - struct rte_eth_dev_info dev_info = { 0 };
> >>> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> >> Hmmm... Unless I miss something this and one above would zero only a
> >> single field - "max_rx_queues"; and would leave the rest uninitialized.
> >> The original code intend to zero the whole struct. The alternative to
> >> the original lines could be usage of memset().
> > As I understand, in that case compiler had to set all non-explicitly 
> > initialised members to 0.
> > So I think we are ok here.
>  Yeah, I guess it does zero-initializes the rest
>  (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
>  don't understand how the above change fixes the error if it complains
>  about the dev_info.driver_name?
> >>> As only 1 field is required, I chose the one which should not be removed
> >>> from this structure in the future.
> >>>
>  What I'm trying to say - the proposed fix is completely unclear and
>  confusing. Think of somebody reading this line in a month from today -
>  he wouldn't get a clue why is it there, why to explicitly set
>  max_rx_queues to zero and leave the rest be zeroed automatically... Why
>  to add such artifacts to the code instead of just zeroing the struct
>  with a memset() and putting a good clear comment above it explaining why
>  we use a memset() and not and initializer?
> >>> We can make it longer yes.
> >>> I think you agree we should avoid extra lines if not needed.
> >>> In this case, when reading "= { .field = 0 }", it seems clear our goal
> >>> is to zero the structure (it is to me).
> >> I'm sorry but it's not clear to me at all since the common C practice
> >> for zeroing the struct would be
> >>
> >> struct st a = {0};
> >>
> >> Like in the lines u are changing. The lines as above are clearly should
> >> not be commented and are absolutely clear.
> >> The lines u are adding on the other hand are absolutely unclear and
> >> confusing outside the gcc bug context. Therefore it should be clearly
> >> stated so in a form of comment. Otherwise somebody (like myself) may see
> >> this and immediately fix it back (as it should be).
> >>
> >>> I thought it is a basic C practice.
> >> I doubt that. ;) Explained above.
> >>
> >>> You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> >>> not going to comment each occurence of this coding style.
> >>> But it must be explained in the coding style document. Agree?
> >> OMG! This is awful! I think everybody agrees that this is a workaround
> >> and has nothing to do with a codding style (it's an opposite to a style
> >> actually). I don't know where this should be explained, frankly.
> > Once we assert we want to support this buggy compiler, the workarounds
> > are automatically parts of the coding style.
> 
> It'd rather not... ;)
> 
> > I don't know how to deal differently with this constraint.
> 
> Add -Wno-missing-braces compilation option for compiler versions below 
> 4.7. U (and me and I guess most other developers) compile DPDK code with 
> a newer compiler thus the code would be properly inspected with these 
> compilers and we may afford to be less restrictive with compilation 
> warnings with legacy compiler versions...

You're right.
I will test it and submit a v2.
Then I could use the above grep command to replace other occurences of this
workaround.

> >> Getting back to the issue - I'm a bit surprised since I use this kind of
> >> initializer ({0}) in a C code for quite a long time - long before 2012.
> >> I'd like to understand what is a problem with this specific gcc version.
> >> This seems to trivial. I'm surprised CentOS has a gcc version with this
> >> kind of bugs.
> > Each day brings its surprise :)

[dpdk-dev] tools brainstorming

[Thomas Monjalon]

2015-04-14 15:52, Bruce Richardson:
> On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
> > When a consensus is done, it must be added with a patch with custom
> > checkpatch addition.
> > 
> My personal feeling is that we should try and keep checkpatch modifications 
> to a
> minimum. Right now, we can use checkpatch as-is from kernel.org, right?

Yes that's something we have to discuss.
It should be preferred to avoid "forking" checkpatch.

At the moment, I'm using this configuration:

options="$options --max-line-length=100"
options="$options --show-types"
options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
SPLIT_STRING,LINE_SPACING,NEW_TYPEDEFS,COMPLEX_MACRO"

linux/scripts/checkpatch.pl $options

I would like to submit a script to run checkpatch with DPDK configuration
when the coding rules are clear.

However, I've already seen some options which are not enough configurable
(don't remember which one). For such corner case, I would see 3 solutions
(from the most to the least desired):
- submit a patch to allow more configuration to kernel.org
- give up automatic handling of corner cases
- maintain a fork in scripts/ directory

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Thomas Monjalon]

2015-04-14 17:59, Vlad Zolotarov:
> On 04/14/15 17:17, Thomas Monjalon wrote:
> > 2015-04-14 16:38, Vlad Zolotarov:
> >> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> >>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>  On 04/14/15 12:31, Thomas Monjalon wrote:
> > -   struct rte_eth_dev_info dev_info = { 0 };
> > +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>  Hmmm... Unless I miss something this and one above would zero only a
>  single field - "max_rx_queues"; and would leave the rest uninitialized.
>  The original code intend to zero the whole struct. The alternative to
>  the original lines could be usage of memset().
> >>> As I understand, in that case compiler had to set all non-explicitly 
> >>> initialised members to 0.
> >>> So I think we are ok here.
> >> Yeah, I guess it does zero-initializes the rest
> >> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
> >> don't understand how the above change fixes the error if it complains
> >> about the dev_info.driver_name?
> > As only 1 field is required, I chose the one which should not be removed
> > from this structure in the future.
> >
> >> What I'm trying to say - the proposed fix is completely unclear and
> >> confusing. Think of somebody reading this line in a month from today -
> >> he wouldn't get a clue why is it there, why to explicitly set
> >> max_rx_queues to zero and leave the rest be zeroed automatically... Why
> >> to add such artifacts to the code instead of just zeroing the struct
> >> with a memset() and putting a good clear comment above it explaining why
> >> we use a memset() and not and initializer?
> > We can make it longer yes.
> > I think you agree we should avoid extra lines if not needed.
> > In this case, when reading "= { .field = 0 }", it seems clear our goal
> > is to zero the structure (it is to me).
> 
> I'm sorry but it's not clear to me at all since the common C practice 
> for zeroing the struct would be
> 
> struct st a = {0};
> 
> Like in the lines u are changing. The lines as above are clearly should 
> not be commented and are absolutely clear.
> The lines u are adding on the other hand are absolutely unclear and 
> confusing outside the gcc bug context. Therefore it should be clearly 
> stated so in a form of comment. Otherwise somebody (like myself) may see 
> this and immediately fix it back (as it should be).
> 
> > I thought it is a basic C practice.
> 
> I doubt that. ;) Explained above.
> 
> > You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> > not going to comment each occurence of this coding style.
> > But it must be explained in the coding style document. Agree?
> 
> OMG! This is awful! I think everybody agrees that this is a workaround 
> and has nothing to do with a codding style (it's an opposite to a style 
> actually). I don't know where this should be explained, frankly.

Once we assert we want to support this buggy compiler, the workarounds
are automatically parts of the coding style.
I don't know how to deal differently with this constraint.

> Getting back to the issue - I'm a bit surprised since I use this kind of 
> initializer ({0}) in a C code for quite a long time - long before 2012. 
> I'd like to understand what is a problem with this specific gcc version. 
> This seems to trivial. I'm surprised CentOS has a gcc version with this 
> kind of bugs.

Each day brings its surprise :)

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Thomas Monjalon]

2015-04-14 17:30, Vlad Zolotarov:
> On 04/14/15 17:17, Thomas Monjalon wrote:
> > 2015-04-14 16:38, Vlad Zolotarov:
> >> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> >>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>  On 04/14/15 12:31, Thomas Monjalon wrote:
> > -   struct rte_eth_dev_info dev_info = { 0 };
> > +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>  Hmmm... Unless I miss something this and one above would zero only a
>  single field - "max_rx_queues"; and would leave the rest uninitialized.
>  The original code intend to zero the whole struct. The alternative to
>  the original lines could be usage of memset().
> >>> As I understand, in that case compiler had to set all non-explicitly 
> >>> initialised members to 0.
> >>> So I think we are ok here.
> >> Yeah, I guess it does zero-initializes the rest
> >> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I
> >> don't understand how the above change fixes the error if it complains
> >> about the dev_info.driver_name?
> > As only 1 field is required, I chose the one which should not be removed
> > from this structure in the future.
> 
> I don't follow - where/why only one field is required? The function u 
> are patching uses "rx_offload_capa" field. Or u mean this gcc version 
> requires only one field? If so, could u, please, provide the errata u 
> are referring, since standard doesn't require any field and {0} is an 
> absolutely legal (and proper) initializer in this case...

Honestly I don't really care what is "legal". The most important is to make
it working with most C compilers with minimal overhead.
You're right about the variable choice: rx_offload_capa is more appropriate.
Are you OK for a v2 replacing max_rx_queues by rx_offload_capa?

> >> What I'm trying to say - the proposed fix is completely unclear and
> >> confusing. Think of somebody reading this line in a month from today -
> >> he wouldn't get a clue why is it there, why to explicitly set
> >> max_rx_queues to zero and leave the rest be zeroed automatically... Why
> >> to add such artifacts to the code instead of just zeroing the struct
> >> with a memset() and putting a good clear comment above it explaining why
> >> we use a memset() and not and initializer?
> > We can make it longer yes.
> > I think you agree we should avoid extra lines if not needed.
> > In this case, when reading "= { .field = 0 }", it seems clear our goal
> > is to zero the structure (it is to me).
> > I thought it is a basic C practice.
> >
> > You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
> > not going to comment each occurence of this coding style.
> > But it must be explained in the coding style document. Agree?
> 

[dpdk-dev] tools brainstorming

[Thomas Monjalon]

2015-04-14 10:38, Neil Horman:
> On Tue, Apr 14, 2015 at 03:21:53PM +0100, Bruce Richardson wrote:
> > On Wed, Apr 08, 2015 at 07:54:40PM +, Butler, Siobhan A wrote:
> > > 
> > > 
> > > > -Original Message-
> > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > Sent: Wednesday, April 8, 2015 5:16 PM
> > > > To: Wiles, Keith; Butler, Siobhan A
> > > > Cc: dev at dpdk.org
> > > > Subject: Re: [dpdk-dev] tools brainstorming
> > > > 
> > > > 2015-04-08 15:53, Wiles, Keith:
> > > > > One of the biggest problems with any style is helping the developer
> > > > > maintain the style. Using some tool does help and I have used astyle
> > > > > before, not bad code formatter. Here is a few that seem to be 
> > > > > reasonable.
> > > > >
> > > > > http://astyle.sourceforge.net/
> > > > >
> > > > > http://uncrustify.sourceforge.net/
> > > > >
> > > > > http://sourceforge.net/projects/gcgreatcode/
> > > > 
> > > > I'm not sure it's a good idea to convert the codebase automatically.
> > > > The coding style must be a reference for new patches and they must be
> > > > automatically checked with a dedicated checkpatch tool.
> > > > By forbidding patches which don't comply, the codebase will be naturally
> > > > converted over time.
> > > > 
> > > > I didn't review this proposal yet.
> > > > My first comment is that it's too long to read :) When a consensus is 
> > > > done, it
> > > > must be added with a patch with custom checkpatch addition.
> > > Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the 
> > > comments supplied so far and post a fresh version tomorrow.
> > > Siobhan 
> > > 
> > 
> > Just wondering here, are we looking to codify what the current predominant 
> > coding
> > style in DPDK *is* or what it *should be*? 
> > 
> > There has been some good discussion on a variety of areas, but if we focus 
> > on
> > initially codifying what's there now, some issues become easier to resolve  
> > -
> > e.g. discussion of commenting style, since only C89 comments are allowed 
> > right now.
> > 
> 
> This is an excellent question.  I think the answer is we should make the style
> what we want it to be. That said, when there is a significant discrepancy 
> behind
> what is wanted and what is, we need to stop and ask ourselves why that exists,
> and what our reasoning is for wanting the change.

Yes the question must be asked.
I think the main goal is to have a consistent style.
As there is already a lot of code with implicit guidelines,
it's simpler to make them official.

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 16:23, Ananyev, Konstantin wrote:
>
>> -Original Message-
>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>> Sent: Tuesday, April 14, 2015 1:52 PM
>> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin
>> Cc: dev at dpdk.org
>> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4
>>
>>
>>
>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
>>> ?dev_info.driver_name?)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
>>> ?dev_info.driver_name?)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
>>> ?ixgbe_recv_pkts_lro_single_alloc?:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
>>> uninitialized in this function
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
>>> uninitialized in this function
>> :D Looks like a gcc bug ;) Both are set and only after that (!!!) used
>> under "!eop" condition.
> Possibly, but we still need to make it build cleanly.

It's clearly - I was just trying to be polite here... ;)
Please, add the comment explaining this initialization so that nobody 
removes these workarounds by mistake...

> Konstantin
>
>>> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>>>
>>> Signed-off-by: Thomas Monjalon 
>>> ---
>>>lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
>>>1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
>>> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> index f1da9ec..a2b8631 100644
>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
>>> **rx_pkts, uint16_t nb_pkts,
>>> bool eop;
>>> struct ixgbe_rx_entry *rxe;
>>> struct ixgbe_rsc_entry *rsc_entry;
>>> -   struct ixgbe_rsc_entry *next_rsc_entry;
>>> -   struct ixgbe_rx_entry *next_rxe;
>>> +   struct ixgbe_rsc_entry *next_rsc_entry = NULL;
>>> +   struct ixgbe_rx_entry *next_rxe = NULL;
>>> struct rte_mbuf *first_seg;
>>> struct rte_mbuf *rxm;
>>> struct rte_mbuf *nmb;
>>> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>> struct ixgbe_rx_queue *rxq;
>>> struct ixgbe_hw *hw;
>>> uint16_t len;
>>> -   struct rte_eth_dev_info dev_info = { 0 };
>>> +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>> struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>>> bool rsc_requested = false;
>>>
>>> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>>>{
>>> struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>>> struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> -   struct rte_eth_dev_info dev_info = { 0 };
>>> +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>> bool rsc_capable = false;
>>> uint16_t i;
>>> uint32_t rdrxctl;

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 16:06, Ananyev, Konstantin wrote:
>
>> -Original Message-
>> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
>> Sent: Tuesday, April 14, 2015 1:49 PM
>> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin
>> Cc: dev at dpdk.org
>> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4
>>
>>
>>
>> On 04/14/15 12:31, Thomas Monjalon wrote:
>>> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
>>> ?dev_info.driver_name?)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
>>> ?dev_info.driver_name?)
>>>
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
>>> ?ixgbe_recv_pkts_lro_single_alloc?:
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
>>> uninitialized in this function
>>> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
>>> uninitialized in this function
>>>
>>> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>>>
>>> Signed-off-by: Thomas Monjalon 
>>> ---
>>>lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
>>>1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
>>> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> index f1da9ec..a2b8631 100644
>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
>>> **rx_pkts, uint16_t nb_pkts,
>>> bool eop;
>>> struct ixgbe_rx_entry *rxe;
>>> struct ixgbe_rsc_entry *rsc_entry;
>>> -   struct ixgbe_rsc_entry *next_rsc_entry;
>>> -   struct ixgbe_rx_entry *next_rxe;
>>> +   struct ixgbe_rsc_entry *next_rsc_entry = NULL;
>>> +   struct ixgbe_rx_entry *next_rxe = NULL;
>>> struct rte_mbuf *first_seg;
>>> struct rte_mbuf *rxm;
>>> struct rte_mbuf *nmb;
>>> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>> struct ixgbe_rx_queue *rxq;
>>> struct ixgbe_hw *hw;
>>> uint16_t len;
>>> -   struct rte_eth_dev_info dev_info = { 0 };
>>> +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>>> struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>>> bool rsc_requested = false;
>>>
>>> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>>>{
>>> struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>>> struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> -   struct rte_eth_dev_info dev_info = { 0 };
>>> +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>> Hmmm... Unless I miss something this and one above would zero only a
>> single field - "max_rx_queues"; and would leave the rest uninitialized.
>> The original code intend to zero the whole struct. The alternative to
>> the original lines could be usage of memset().
> As I understand, in that case compiler had to set all non-explicitly 
> initialised members to 0.
> So I think we are ok here.

Yeah, I guess it does zero-initializes the rest 
(https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I 
don't understand how the above change fixes the error if it complains 
about the dev_info.driver_name?

What I'm trying to say - the proposed fix is completely unclear and 
confusing. Think of somebody reading this line in a month from today - 
he wouldn't get a clue why is it there, why to explicitly set 
max_rx_queues to zero and leave the rest be zeroed automatically... Why 
to add such artifacts to the code instead of just zeroing the struct 
with a memset() and putting a good clear comment above it explaining why 
we use a memset() and not and initializer?

>   
>>> bool rsc_capable = false;
>>> uint16_t i;
>>> uint32_t rdrxctl;

[dpdk-dev] tools brainstorming

[Bruce Richardson]

On Wed, Apr 08, 2015 at 11:16:03AM -0700, Stephen Hemminger wrote:
> Thanks for doing this, it is a great start.
> I admit strong bias towards Linux kernel style.
> 
> Could you use one of the standard markup styles so that it could get put in 
> documentation?
> 
>  
> > License Header
> > --
> 
> I prefer the file just say that it is BSD or GPL and refer to license files 
> in the
> package. That way if something has to change it doesn't need a massive 
> license sweep
> 
>  
> > 
> > Macros
> > 
> > Do not ``#define`` or declare names in the implementation namespace except 
> > for implementing application interfaces. 
> > 
> > The names of ``unsafe`` macros (ones that have side effects), and the names 
> > of macros for manifest constants, are all in uppercase. 
> > 
> > The expansions of expression-like macros are either a single token or have 
> > outer parentheses. If a macro is an inline expansion of a function, 
> > the function name is all in lowercase and the macro has the same name all 
> > in uppercase. Right-justify the backslashes; 
> > it makes it easier to read. If the macro encapsulates a compound statement, 
> > enclose it in a do loop, so that it can be used safely in if statements. 
> > Any final statement-terminating semicolon should be supplied by the macro 
> > invocation rather than the macro, to make parsing easier for 
> > pretty-printers and editors. 
> >  #define MACRO(x, y) do {\
> >  variable = (x) + (y);   \
> >  (y) += 2;   \
> >  }while (0)
> ^ bad whitespace
> 
> it is important that all examples in documentation are perfect.
> 
> 
> > C Function Definition, Declaration and Use
> > 
> > Prototypes
> > 
> > It is recommended, but not required that all functions are prototyped 
> > somewhere. 
> > 
> > Any function prototypes for private functions (that is, functions not used 
> > elsewhere) go at the top of the first source module. Functions 
> > local to one source module should be declared static. 
> 
> I find prototypes for private functions to be redundant and error prone.
> The do nothing. Better to just put private functions in the correct order.
> 
> 
> You also need to raise the issue that all global names need to be prefaced by 
> a unique string.
> I see places in drivers where global names leak out causing possible later 
> name collision.
> 
+1 to both.

> > Definitions
> > ---
> > 
> > The function type should be on a line by itself preceding the function. The 
> > opening brace of the function body should be on a line by itself. 
> >  static char *
> >  function(int a1, int a2, float fl, int a4)
> >  {
> 
> Not a big fan of that style. Prefer it on same line.
> 
> 
> > 
> > Indentation is a hard tab, that is, a tab character, not a sequence of 
> > spaces. 
> 
> Also no spaces before tabs.
> 
> > NOTE General rule in DPDK, use tabs for indentation, spaces for alignment. 
> > If you have to wrap a long statement, put the operator at the end of the 
> > line, and indent again. For control statements (if, while, etc.), 
> > it is recommended that the next line be indented by two tabs, rather than 
> > one, to prevent confusion as to whether the second line of the 
> > control statement forms part of the statement body or not. For non-control 
> > statements, this issue does not apply, so they can be indented 
> > by a single tab. However, a two-tab indent is recommended in this case also 
> > to keep consistency across all statement types. 
> >  while (really_long_variable_name_1 == really_long_variable_name_2 &&
> >  var3 == var4){
> >  x = y + z;  /* control stmt body lines up with second line of */
> >  a = b + c;  /* control statement itself if single indent used */
> >  }
> >  
> >  if (really_long_variable_name_1 == really_long_variable_name_2 &&
> >  var3 == var4){  /* two tabs used */
> 
> No. Should line up with really_long_variable_name_1
> 

I disagree with that. For a couple of reasons:
*. It means using spaces as well as tabs for indentation, while I think either
one or the other should be used, not both.
*. For anyone using a 4-character tab-stop display, the var3 line will line up
visually with the body of the block making it look like the body, rather than
part of the condition. For anyone using an 8-character tab, the same effect
will be got with a while statement with a couple of opening braces. By using
two tabs, we guarantee that the line continuation never lines up with the 
body of the block.

> >  x = y + z;  /* statement body no longer lines up */
> >  a = b + c;
> >  }
> >  
> >  z = a + really + long + statement + that + needs +
> >  two + lines + gets + indented + on + the + 
> >  second + and + subsequent + lines;
> > 
> > 
> > Do not add whitespace at the end of a line. 
> > 
> > Closing and opening braces go on the same 

[dpdk-dev] [PATCH] enic: set correct port ID in received mbufs

[Adrien Mazarguil]

This field is not supposed to contain the RX queue index. Applications can
rely on it to determine the port a given mbuf comes from.

Signed-off-by: Adrien Mazarguil 
---
 lib/librte_pmd_enic/enic.h| 1 +
 lib/librte_pmd_enic/enic_ethdev.c | 1 +
 lib/librte_pmd_enic/enic_main.c   | 4 ++--
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/librte_pmd_enic/enic.h b/lib/librte_pmd_enic/enic.h
index a50bff1..0eba334 100644
--- a/lib/librte_pmd_enic/enic.h
+++ b/lib/librte_pmd_enic/enic.h
@@ -99,6 +99,7 @@ struct enic {
struct vnic_dev_bar bar0;
struct vnic_dev *vdev;

+   unsigned int port_id;
struct rte_eth_dev *rte_dev;
struct enic_fdir fdir;
char bdf_name[ENICPMD_BDF_LENGTH];
diff --git a/lib/librte_pmd_enic/enic_ethdev.c 
b/lib/librte_pmd_enic/enic_ethdev.c
index 3e53f86..a319e1e 100644
--- a/lib/librte_pmd_enic/enic_ethdev.c
+++ b/lib/librte_pmd_enic/enic_ethdev.c
@@ -567,6 +567,7 @@ static int eth_enicpmd_dev_init(struct rte_eth_dev *eth_dev)

ENICPMD_FUNC_TRACE();

+   enic->port_id = eth_dev->data->port_id;
enic->rte_dev = eth_dev;
eth_dev->dev_ops = &enicpmd_eth_dev_ops;
eth_dev->rx_pkt_burst = &enicpmd_recv_pkts;
diff --git a/lib/librte_pmd_enic/enic_main.c b/lib/librte_pmd_enic/enic_main.c
index 0e40d46..15313c2 100644
--- a/lib/librte_pmd_enic/enic_main.c
+++ b/lib/librte_pmd_enic/enic_main.c
@@ -344,7 +344,7 @@ static int enic_rq_alloc_buf(struct vnic_rq *rq)
hdr_mbuf->data_off = RTE_PKTMBUF_HEADROOM;

hdr_mbuf->nb_segs = 2;
-   hdr_mbuf->port = rq->index;
+   hdr_mbuf->port = enic->port_id;
hdr_mbuf->next = mbuf;

dma_addr = (dma_addr_t)
@@ -359,7 +359,7 @@ static int enic_rq_alloc_buf(struct vnic_rq *rq)
type = RQ_ENET_TYPE_NOT_SOP;
} else {
mbuf->nb_segs = 1;
-   mbuf->port = rq->index;
+   mbuf->port = enic->port_id;
}

mbuf->data_off = RTE_PKTMBUF_HEADROOM;
-- 
2.1.0

[dpdk-dev] tools brainstorming

[Wiles, Keith]

On 4/14/15, 10:24 AM, "Thomas Monjalon"  wrote:

>2015-04-14 15:52, Bruce Richardson:
>> On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
>> > When a consensus is done, it must be added with a patch with custom
>> > checkpatch addition.
>> > 
>> My personal feeling is that we should try and keep checkpatch
>>modifications to a
>> minimum. Right now, we can use checkpatch as-is from kernel.org, right?
>
>Yes that's something we have to discuss.
>It should be preferred to avoid "forking" checkpatch.
>
>At the moment, I'm using this configuration:
>
>   options="$options --max-line-length=100"
>   options="$options --show-types"
>   options="$options --ignore=LINUX_VERSION_CODE,FILE_PATH_CHANGES,\
>   VOLATILE,PREFER_PACKED,PREFER_ALIGNED,PREFER_PRINTF,\
>   SPLIT_STRING,LINE_SPACING,NEW_TYPEDEFS,COMPLEX_MACRO"
>
>   linux/scripts/checkpatch.pl $options
>
>I would like to submit a script to run checkpatch with DPDK configuration
>when the coding rules are clear.
>
>However, I've already seen some options which are not enough configurable
>(don't remember which one). For such corner case, I would see 3 solutions
>(from the most to the least desired):
>   - submit a patch to allow more configuration to kernel.org
>   - give up automatic handling of corner cases
>   - maintain a fork in scripts/ directory
Here is the next solution
- Stop using checkpatch and use a real tool for formatting code instead.
If someone uses a tool before commit, then create the patch which does not
require checkpatch.
Most of these tools can define an output file or they leave behind the
original file as a backup or we can see if they have a non-modify mode and
just points out the problems. As in astyle '--dry-run' can be used, plus
it saves the original file as X.orig or you can change the .orig to
your own value.
>

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Thomas Monjalon]

2015-04-14 16:38, Vlad Zolotarov:
> On 04/14/15 16:06, Ananyev, Konstantin wrote:
> > From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
> >> On 04/14/15 12:31, Thomas Monjalon wrote:
> >>> - struct rte_eth_dev_info dev_info = { 0 };
> >>> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> >> 
> >> Hmmm... Unless I miss something this and one above would zero only a
> >> single field - "max_rx_queues"; and would leave the rest uninitialized.
> >> The original code intend to zero the whole struct. The alternative to
> >> the original lines could be usage of memset().
> > 
> > As I understand, in that case compiler had to set all non-explicitly 
> > initialised members to 0.
> > So I think we are ok here.
> 
> Yeah, I guess it does zero-initializes the rest 
> (https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html) however I 
> don't understand how the above change fixes the error if it complains 
> about the dev_info.driver_name?

As only 1 field is required, I chose the one which should not be removed
from this structure in the future.

> What I'm trying to say - the proposed fix is completely unclear and 
> confusing. Think of somebody reading this line in a month from today - 
> he wouldn't get a clue why is it there, why to explicitly set 
> max_rx_queues to zero and leave the rest be zeroed automatically... Why 
> to add such artifacts to the code instead of just zeroing the struct 
> with a memset() and putting a good clear comment above it explaining why 
> we use a memset() and not and initializer?

We can make it longer yes.
I think you agree we should avoid extra lines if not needed.
In this case, when reading "= { .field = 0 }", it seems clear our goal
is to zero the structure (it is to me).
I thought it is a basic C practice.

You should try "git grep '\.[^ ]\+ *= *0 *}'" to be convinced that we are
not going to comment each occurence of this coding style.
But it must be explained in the coding style document. Agree?

[dpdk-dev] [PATCH v5 1/8] Move common functions in eal_thread.c

[Thomas Monjalon]

Hi Ravi,

2015-04-09 12:40, Ravi Kerur:
> --- a/lib/librte_eal/common/eal_common_thread.c
> +++ b/lib/librte_eal/common/eal_common_thread.c
[...]
> +#ifdef RTE_EXEC_ENV_BSDAPP
> +#include 
> +#include 
> +#else /* RTE_EXEC_ENV_BSDAPP */
>  #include 
> +#endif /* RTE_EXEC_ENV_BSDAPP */
[...]
> +#ifdef RTE_EXEC_ENV_BSDAPP
> + RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%p;cpuset=[%s%s])\n",
> + lcore_id, thread_id, cpuset, ret == 0 ? "" : "...");
> +#else /* RTE_EXEC_ENV_BSDAPP */
> + RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%x;cpuset=[%s%s])\n",
> + lcore_id, (int)thread_id, cpuset, ret == 0 ? "" : "...");
> +#endif /* RTE_EXEC_ENV_BSDAPP */

These lines should stay in bsdapp and linuxapp directory.
You can add a new function to eal_thread.h to format the thread id,
so you'll be able to use %s in generic log above.

[dpdk-dev] tools brainstorming

[Bruce Richardson]

On Tue, Apr 14, 2015 at 04:47:47PM +0200, Thomas Monjalon wrote:
> 2015-04-14 10:38, Neil Horman:
> > On Tue, Apr 14, 2015 at 03:21:53PM +0100, Bruce Richardson wrote:
> > > On Wed, Apr 08, 2015 at 07:54:40PM +, Butler, Siobhan A wrote:
> > > > 
> > > > 
> > > > > -Original Message-
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > > > Sent: Wednesday, April 8, 2015 5:16 PM
> > > > > To: Wiles, Keith; Butler, Siobhan A
> > > > > Cc: dev at dpdk.org
> > > > > Subject: Re: [dpdk-dev] tools brainstorming
> > > > > 
> > > > > 2015-04-08 15:53, Wiles, Keith:
> > > > > > One of the biggest problems with any style is helping the developer
> > > > > > maintain the style. Using some tool does help and I have used astyle
> > > > > > before, not bad code formatter. Here is a few that seem to be 
> > > > > > reasonable.
> > > > > >
> > > > > > http://astyle.sourceforge.net/
> > > > > >
> > > > > > http://uncrustify.sourceforge.net/
> > > > > >
> > > > > > http://sourceforge.net/projects/gcgreatcode/
> > > > > 
> > > > > I'm not sure it's a good idea to convert the codebase automatically.
> > > > > The coding style must be a reference for new patches and they must be
> > > > > automatically checked with a dedicated checkpatch tool.
> > > > > By forbidding patches which don't comply, the codebase will be 
> > > > > naturally
> > > > > converted over time.
> > > > > 
> > > > > I didn't review this proposal yet.
> > > > > My first comment is that it's too long to read :) When a consensus is 
> > > > > done, it
> > > > > must be added with a patch with custom checkpatch addition.
> > > > Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with 
> > > > the comments supplied so far and post a fresh version tomorrow.
> > > > Siobhan 
> > > > 
> > > 
> > > Just wondering here, are we looking to codify what the current 
> > > predominant coding
> > > style in DPDK *is* or what it *should be*? 
> > > 
> > > There has been some good discussion on a variety of areas, but if we 
> > > focus on
> > > initially codifying what's there now, some issues become easier to 
> > > resolve  -
> > > e.g. discussion of commenting style, since only C89 comments are allowed 
> > > right now.
> > > 
> > 
> > This is an excellent question.  I think the answer is we should make the 
> > style
> > what we want it to be. That said, when there is a significant discrepancy 
> > behind
> > what is wanted and what is, we need to stop and ask ourselves why that 
> > exists,
> > and what our reasoning is for wanting the change.
> 
> Yes the question must be asked.
> I think the main goal is to have a consistent style.
> As there is already a lot of code with implicit guidelines,
> it's simpler to make them official.
> 
Sounds good to me. Let's document what we have, then evolve it as necessary. :-)

[dpdk-dev] tools brainstorming

[Bruce Richardson]

On Wed, Apr 08, 2015 at 06:16:12PM +0200, Thomas Monjalon wrote:
> 2015-04-08 15:53, Wiles, Keith:
> > One of the biggest problems with any style is helping the developer
> > maintain the style. Using some tool does help and I have used astyle
> > before, not bad code formatter. Here is a few that seem to be reasonable.
> > 
> > http://astyle.sourceforge.net/
> > 
> > http://uncrustify.sourceforge.net/
> > 
> > http://sourceforge.net/projects/gcgreatcode/
> 
> I'm not sure it's a good idea to convert the codebase automatically.
> The coding style must be a reference for new patches and they must be
> automatically checked with a dedicated checkpatch tool.
> By forbidding patches which don't comply, the codebase will be naturally
> converted over time.
> 

I'd like to see us document the existing style as much as possible before 
changing
it. That saves any conversion issues. In cases where multiple styles are used,
we can initially go with the more prevalent one.

> I didn't review this proposal yet.
> My first comment is that it's too long to read :)
> When a consensus is done, it must be added with a patch with custom
> checkpatch addition.
> 
My personal feeling is that we should try and keep checkpatch modifications to a
minimum. Right now, we can use checkpatch as-is from kernel.org, right?

/Bruce

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 12:31, Thomas Monjalon wrote:
> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
> ?dev_info.driver_name?)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
> ?dev_info.driver_name?)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
> ?ixgbe_recv_pkts_lro_single_alloc?:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
> uninitialized in this function
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
> uninitialized in this function

:D Looks like a gcc bug ;) Both are set and only after that (!!!) used 
under "!eop" condition.

>
> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>
> Signed-off-by: Thomas Monjalon 
> ---
>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index f1da9ec..a2b8631 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
> **rx_pkts, uint16_t nb_pkts,
>   bool eop;
>   struct ixgbe_rx_entry *rxe;
>   struct ixgbe_rsc_entry *rsc_entry;
> - struct ixgbe_rsc_entry *next_rsc_entry;
> - struct ixgbe_rx_entry *next_rxe;
> + struct ixgbe_rsc_entry *next_rsc_entry = NULL;
> + struct ixgbe_rx_entry *next_rxe = NULL;
>   struct rte_mbuf *first_seg;
>   struct rte_mbuf *rxm;
>   struct rte_mbuf *nmb;
> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>   struct ixgbe_rx_queue *rxq;
>   struct ixgbe_hw *hw;
>   uint16_t len;
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>   struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>   bool rsc_requested = false;
>   
> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>   {
>   struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>   struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>   bool rsc_capable = false;
>   uint16_t i;
>   uint32_t rdrxctl;

[dpdk-dev] tools brainstorming

[Bruce Richardson]

On Wed, Apr 08, 2015 at 02:16:55PM +, Wiles, Keith wrote:
> 
> 
> On 4/8/15, 5:43 AM, "Butler, Siobhan A"  wrote:
> 
> >Hi all,
> >To add to the tools brainstorming - I propose we use the following Coding
> >Standards as the basis of guidelines on coding style going forward.
> >The style outlined below is in alignment with the current convention used
> >for the majority of the project.
> >Any thoughts/suggestions or feedback welcome.
> >Thanks
> >Siobhan :)
> >
> >
> >
> >
> >Coding Style
> >~~
> >
> >Description
> >---
> >
> >This document specifies the preferred style for source files in the DPDK
> >source tree. 
> >It is based on the Linux Kernel coding guidelines and the FreeBSD 7.2
> >Kernel Developer's Manual (see man style(9)),
> >but was heavily modified for the needs of the DPDK. Many of the style
> >rules are implicit in the examples.
> >Be careful to check the examples before assuming that style is silent on
> >an issue. 
> >
> >General Guidelines
> >--
> >
> >The rules and guidelines given in this document cannot cover every
> >situation, so the following general guidelines should be used as a
> >fallback: 
> >The code style should be consistent within each individual file, and
> >within each file in a given directory or module - in the case of creating
> >new files 
> >The primary reason for coding standards is to increase code readability
> >and comprehensibility, therefore always use whatever option will make the
> >code easiest to read.
> >
> >The following more specific recommendations apply to all sections, both
> >for C and assembly code:
> >Line length is recommended to be not more than 80 characters, including
> >comments. [Tab stop size should be assumed to be at least 4-characters
> >wide] 
> >Indentation should be to no more than 3 levels deep.
> >NOTE The above are recommendations, and not hard limits. However, it is
> >expected that the recommendations should be followed in all but the
> >rarest situations.
> >C Comment Style
> >
> >Usual Comments
> >--
> >
> >These comments should be used in normal cases. To document a public API,
> >a doxygen-like format must be used: refer to Doxygen Documentation.
> > /*
> >  * VERY important single-line comments look like this.
> >  */
> > 
> > /* Most single-line comments look like this. */
> > 
> > /*
> >  * Multi-line comments look like this.  Make them real sentences. Fill
> >  * them so they look like real paragraphs.
> >  */
> 
> Did you mean to have the ?*? aligned, if so good, if not then it does not
> make sense to not align them. The indent of one space here does not help
> convey any information IMO.
> >
> >License Header
> >--
> >
> >Each file should begin with a special comment tag which will contain the
> >appropriate copyright and license for the file (Generally BSD License).
> >After any copyright header, a blank line should be left before any other
> >contents, e.g. include statements in a C file.
> >
> >C Preprocessor Directives
> >-
> >
> >Header Includes
> >
> >In DPDK sources, the include files should be ordered as following:
> > libc includes (system includes first)
> > DPDK EAL includes
> > DPDK misc libraries includes
> > application-specific includes
> >
> >Example: 
> > #include 
> > #include 
> > 
> > #include 
> > 
> > #include 
> > #include 
> > 
> > #include "application.h"
> >
> >
> >Global pathnames are defined in . Pathnames local to the program
> >go in "pathnames.h" in the local directory.
> > #include 
> >
> >
> >Leave another blank line before the user include files.
> > #include "pathnames.h" /* Local includes in double quotes. */
> >
> >NOTE Please avoid, as much as possible, including headers from other
> >headers file. Doing so should be properly explained and justified.
> >Headers should be protected against multiple inclusion with the usual:
> > #ifndef _FILE_H_
> > #define _FILE_H_
> > 
> > /* Code */
> > 
> > #endif /* _FILE_H_ */
> >
> >
> >Macros
> >
> >Do not ``#define`` or declare names in the implementation namespace
> >except for implementing application interfaces.
> >
> >The names of ``unsafe`` macros (ones that have side effects), and the
> >names of macros for manifest constants, are all in uppercase.
> >
> >The expansions of expression-like macros are either a single token or
> >have outer parentheses. If a macro is an inline expansion of a function,
> >the function name is all in lowercase and the macro has the same name all
> >in uppercase. Right-justify the backslashes;
> >it makes it easier to read. If the macro encapsulates a compound
> >statement, enclose it in a do loop, so that it can be used safely in if
> >statements. 
> >Any final statement-terminating semicolon should be supplied by the macro
> >invocation rather than the macro, to make parsing easier for
> >pretty-printers and editors.
> > #define MACRO(x, y) do {\
> > variable = (x) + (

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Vlad Zolotarov]

On 04/14/15 12:31, Thomas Monjalon wrote:
> With GCC 4.4.7 from CentOS 6.5, the following errors arise:
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
> ?dev_info.driver_name?)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
> ?dev_info.driver_name?)
>
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
> ?ixgbe_recv_pkts_lro_single_alloc?:
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
> uninitialized in this function
> lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
> uninitialized in this function
>
> Fixes: 8eecb3295aed ("ixgbe: add LRO support")
>
> Signed-off-by: Thomas Monjalon 
> ---
>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
> b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index f1da9ec..a2b8631 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
> **rx_pkts, uint16_t nb_pkts,
>   bool eop;
>   struct ixgbe_rx_entry *rxe;
>   struct ixgbe_rsc_entry *rsc_entry;
> - struct ixgbe_rsc_entry *next_rsc_entry;
> - struct ixgbe_rx_entry *next_rxe;
> + struct ixgbe_rsc_entry *next_rsc_entry = NULL;
> + struct ixgbe_rx_entry *next_rxe = NULL;
>   struct rte_mbuf *first_seg;
>   struct rte_mbuf *rxm;
>   struct rte_mbuf *nmb;
> @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>   struct ixgbe_rx_queue *rxq;
>   struct ixgbe_hw *hw;
>   uint16_t len;
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
>   struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
>   bool rsc_requested = false;
>   
> @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
>   {
>   struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
>   struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> - struct rte_eth_dev_info dev_info = { 0 };
> + struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };

Hmmm... Unless I miss something this and one above would zero only a 
single field - "max_rx_queues"; and would leave the rest uninitialized.
The original code intend to zero the whole struct. The alternative to 
the original lines could be usage of memset().

>   bool rsc_capable = false;
>   uint16_t i;
>   uint32_t rdrxctl;

[dpdk-dev] tools brainstorming

[Bruce Richardson]

On Wed, Apr 08, 2015 at 07:54:40PM +, Butler, Siobhan A wrote:
> 
> 
> > -Original Message-
> > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > Sent: Wednesday, April 8, 2015 5:16 PM
> > To: Wiles, Keith; Butler, Siobhan A
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] tools brainstorming
> > 
> > 2015-04-08 15:53, Wiles, Keith:
> > > One of the biggest problems with any style is helping the developer
> > > maintain the style. Using some tool does help and I have used astyle
> > > before, not bad code formatter. Here is a few that seem to be reasonable.
> > >
> > > http://astyle.sourceforge.net/
> > >
> > > http://uncrustify.sourceforge.net/
> > >
> > > http://sourceforge.net/projects/gcgreatcode/
> > 
> > I'm not sure it's a good idea to convert the codebase automatically.
> > The coding style must be a reference for new patches and they must be
> > automatically checked with a dedicated checkpatch tool.
> > By forbidding patches which don't comply, the codebase will be naturally
> > converted over time.
> > 
> > I didn't review this proposal yet.
> > My first comment is that it's too long to read :) When a consensus is done, 
> > it
> > must be added with a patch with custom checkpatch addition.
> Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the 
> comments supplied so far and post a fresh version tomorrow.
> Siobhan 
> 

Just wondering here, are we looking to codify what the current predominant 
coding
style in DPDK *is* or what it *should be*? 

There has been some good discussion on a variety of areas, but if we focus on
initially codifying what's there now, some issues become easier to resolve  -
e.g. discussion of commenting style, since only C89 comments are allowed right 
now.

/Bruce

[dpdk-dev] rte_ring's dequeue appears to be slow

[Dor Green]

Dequeuing is done in bulk (that's what shows as CPU consuming, as
well). Enqueuing is not due to some constraint we have.

It seemed likely that the dequeue function is falsely blamed for
taking up CPU, but in my tests there are constant incoming packets so
I don't see when it will poll and receive no packets.
Any other ideas to check?

On Mon, Apr 6, 2015 at 11:43 PM, Stephen Hemminger
 wrote:
> On Mon, 6 Apr 2015 15:18:21 +0300
> Dor Green  wrote:
>
>> I have an app which captures packets on a single core and then passes
>> to multiple workers on different lcores, using the ring queues.
>>
>> While I manage to capture packets at 10Gbps, when I send it to the
>> processing lcores there is substantial packet loss. At first I figured
>> it's the processing I do on the packets and optimized that, which did
>> help it a little but did not alleviate the problem.
>>
>> I used Intel VTune amplifier to profile the program, and on all
>> profiling checks that I did there, the majority of the time in the
>> program is spent in "__rte_ring_sc_do_dequeue" (about 70%). I was
>> wondering if anyone can tell me how to optimize this, or if I'm using
>> the queues incorrectly, or maybe even doing the profiling wrong
>> (because I do find it weird that this dequeuing is so slow).
>>
>> My program architecture is as follows (replaced consts with actual values):
>>
>> A queue is created for each processing lcore:
>>   rte_ring_create(qname, swsize, NUMA_SOCKET, 1024*1024,
>> RING_F_SP_ENQ | RING_F_SC_DEQ);
>>
>> The processing core enqueues packets one by one, to each of the queues
>> (the packet burst size is 256):
>>  rte_ring_sp_enqueue(lc[queue_index].queue, (void *const)pkts[i]);
>>
>> Which are then dequeued in bulk in the processor lcores:
>>  rte_ring_sc_dequeue_bulk(lc->queue, (void**) &mbufs, 128);
>>
>> I'm using 16 1GB hugepages, running the new 2.0 version. If there's
>> any further info required about the program, let me know.
>>
>> Thank you.
>
> First off, make sure you are enqueuing and dequeuing in bursts
> if possible. That saves a lot of the overhead.
>
> Also, with polling applications, the dequeue function can be
> falsely blamed for taking CPU, if most of the time the poll does
> not succeed in finding any data.

[dpdk-dev] [PATCH v5 1/8] Move common functions in eal_thread.c

[Ravi Kerur]

On Tue, Apr 14, 2015 at 6:59 AM, Thomas Monjalon 
wrote:

> Hi Ravi,
>
> 2015-04-09 12:40, Ravi Kerur:
> > --- a/lib/librte_eal/common/eal_common_thread.c
> > +++ b/lib/librte_eal/common/eal_common_thread.c
> [...]
> > +#ifdef RTE_EXEC_ENV_BSDAPP
> > +#include 
> > +#include 
> > +#else /* RTE_EXEC_ENV_BSDAPP */
> >  #include 
> > +#endif /* RTE_EXEC_ENV_BSDAPP */
> [...]
> > +#ifdef RTE_EXEC_ENV_BSDAPP
> > + RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%p;cpuset=[%s%s])\n",
> > + lcore_id, thread_id, cpuset, ret == 0 ? "" : "...");
> > +#else /* RTE_EXEC_ENV_BSDAPP */
> > + RTE_LOG(DEBUG, EAL, "lcore %u is ready (tid=%x;cpuset=[%s%s])\n",
> > + lcore_id, (int)thread_id, cpuset, ret == 0 ? "" : "...");
> > +#endif /* RTE_EXEC_ENV_BSDAPP */
>
> These lines should stay in bsdapp and linuxapp directory.
> You can add a new function to eal_thread.h to format the thread id,
> so you'll be able to use %s in generic log above.
>

Thomas, sure will make the changes. I will wait for additional comments if
any for other patches and send v6 together.

Thanks.

[dpdk-dev] [PATCH 1/5] bond: use existing enslaved device queues

[Wodkowski, PawelX]

> 
> Pawel,
> 
> I generally test things I've just built using virtio devices and calling
> rte_eth_tx_queue_setup() more than once for a given queue id fails.
> However, it seems that most PMDs allow re-allocating device queues while
> virtio does not (xenvirt also seems to lack this functionality), so I
> don't think my approach here is right.  I'll remove this patch when I
> send the next version of this series.
> 
> Thanks,
> 
> Eric

Maybe you should rise this as separate issue maintainers of these drivers?

-- 
Pawel

[dpdk-dev] [PATCH] Clean up rte_memcpy.h file

[Ravi Kerur]

Remove unnecessary type casting in functions.
Use loop to adjust offset during copy instead of separate invocations.

Signed-off-by: Ravi Kerur 
---
 .../common/include/arch/x86/rte_memcpy.h   | 317 ++---
 1 file changed, 151 insertions(+), 166 deletions(-)

diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h 
b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
index 6a57426..921e990 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
@@ -106,8 +106,10 @@ rte_mov32(uint8_t *dst, const uint8_t *src)
 static inline void
 rte_mov64(uint8_t *dst, const uint8_t *src)
 {
-   rte_mov32((uint8_t *)dst + 0 * 32, (const uint8_t *)src + 0 * 32);
-   rte_mov32((uint8_t *)dst + 1 * 32, (const uint8_t *)src + 1 * 32);
+   uint8_t i;
+
+   for (i = 0; i < 2; i++)
+   rte_mov32(dst + i * 32, src + i * 32);
 }

 /**
@@ -117,10 +119,10 @@ rte_mov64(uint8_t *dst, const uint8_t *src)
 static inline void
 rte_mov128(uint8_t *dst, const uint8_t *src)
 {
-   rte_mov32((uint8_t *)dst + 0 * 32, (const uint8_t *)src + 0 * 32);
-   rte_mov32((uint8_t *)dst + 1 * 32, (const uint8_t *)src + 1 * 32);
-   rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
-   rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
+   uint8_t i;
+
+   for (i = 0; i < 4; i++)
+   rte_mov32(dst + i * 32, src + i * 32);
 }

 /**
@@ -130,14 +132,10 @@ rte_mov128(uint8_t *dst, const uint8_t *src)
 static inline void
 rte_mov256(uint8_t *dst, const uint8_t *src)
 {
-   rte_mov32((uint8_t *)dst + 0 * 32, (const uint8_t *)src + 0 * 32);
-   rte_mov32((uint8_t *)dst + 1 * 32, (const uint8_t *)src + 1 * 32);
-   rte_mov32((uint8_t *)dst + 2 * 32, (const uint8_t *)src + 2 * 32);
-   rte_mov32((uint8_t *)dst + 3 * 32, (const uint8_t *)src + 3 * 32);
-   rte_mov32((uint8_t *)dst + 4 * 32, (const uint8_t *)src + 4 * 32);
-   rte_mov32((uint8_t *)dst + 5 * 32, (const uint8_t *)src + 5 * 32);
-   rte_mov32((uint8_t *)dst + 6 * 32, (const uint8_t *)src + 6 * 32);
-   rte_mov32((uint8_t *)dst + 7 * 32, (const uint8_t *)src + 7 * 32);
+   uint8_t i;
+
+   for (i = 0; i < 8; i++)
+   rte_mov32(dst + i * 32, src + i * 32);
 }

 /**
@@ -147,16 +145,19 @@ rte_mov256(uint8_t *dst, const uint8_t *src)
 static inline void
 rte_mov64blocks(uint8_t *dst, const uint8_t *src, size_t n)
 {
-   __m256i ymm0, ymm1;
+   __m256i ymm;
+   uint8_t i;

while (n >= 64) {
-   ymm0 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 0 * 32));
+
+   for (i = 0; i < 2; i++) {
+   ymm = _mm256_loadu_si256((const __m256i *)(src + i * 
32));
+   _mm256_storeu_si256((__m256i *)(dst + i * 32), ymm);
+   }
+
n -= 64;
-   ymm1 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 1 * 32));
-   src = (const uint8_t *)src + 64;
-   _mm256_storeu_si256((__m256i *)((uint8_t *)dst + 0 * 32), ymm0);
-   _mm256_storeu_si256((__m256i *)((uint8_t *)dst + 1 * 32), ymm1);
-   dst = (uint8_t *)dst + 64;
+   src = src + 64;
+   dst = dst + 64;
}
 }

@@ -167,37 +168,30 @@ rte_mov64blocks(uint8_t *dst, const uint8_t *src, size_t 
n)
 static inline void
 rte_mov256blocks(uint8_t *dst, const uint8_t *src, size_t n)
 {
-   __m256i ymm0, ymm1, ymm2, ymm3, ymm4, ymm5, ymm6, ymm7;
+   __m256i ymm;
+   uint8_t i;

while (n >= 256) {
-   ymm0 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 0 * 32));
+
+   for (i = 0; i < 8; i++) {
+   ymm = _mm256_loadu_si256((const __m256i *)(src + i * 
32));
+   _mm256_storeu_si256((__m256i *)(dst + i * 32), ymm);
+   }
+
n -= 256;
-   ymm1 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 1 * 32));
-   ymm2 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 2 * 32));
-   ymm3 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 3 * 32));
-   ymm4 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 4 * 32));
-   ymm5 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 5 * 32));
-   ymm6 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 6 * 32));
-   ymm7 = _mm256_loadu_si256((const __m256i *)((const uint8_t 
*)src + 7 * 32));
-   src = (const uint8_t *)src + 256;
-   _mm256_storeu_si256((__m256i *)((uint8_t *)dst + 0 * 32), ymm0);
-   _mm256_storeu_si256((__m256i *)((uint8_t *)dst + 1 * 32), ymm1);
-   _mm256_storeu_si256((__m256i *)((uint8_t *)dst + 2 * 32), ymm2);
-   _mm256_storeu_si2

[dpdk-dev] [PATCH] Cleanup rte_memcpy.h

[Ravi Kerur]

rte_memcpy.h has
sperfluous type casting in several functions, remove those unnecessary casting.
while copying separate invocations of functions with changing offset, instead
offset can be calculated with loop.

Testing:
Compared code generated with and without changes with following gcc commands

gcc -O3 -m64 -S 

found no difference.

Tested on Ubuntu x86_64 (x86_64-native-linuxapp-gcc) with "make test"

Overall tests passed matches baseline.

Secondly memcpy performace tests take similar amount of time to finish.

/**With changes*/
Start memcpy_perf: Success   [00m 00s]
Memcpy performance autotest:   Success   [09m 36s] [17m 45s]
/**Without changes**/
Start memcpy_perf: Success   [00m 00s]
Memcpy performance autotest:   Success   [09m 35s] [13m 57s]

Ravi Kerur (1):
  Clean up rte_memcpy.h file

 .../common/include/arch/x86/rte_memcpy.h   | 317 ++---
 1 file changed, 151 insertions(+), 166 deletions(-)

-- 
1.9.1

[dpdk-dev] rte_memcpy.h

[Ravi Kerur]

DPDK team,

I am looking at rte_memcpy.h implementation and I wasn't sure whether
coding in that file is done for any specific reason. I see superfluous type
casting in functions and instead of using loop for offset changes during
copy, separate invocation (same function) is done repeatedly.

I modified the code to remove unnecessary type casting and used loop for
offset changes. I compared the code generated by gcc (4.8.2) and in both
cases it looked same. In addition, "make test" for memcpy performance gave
similar results. I will send out a patch so you can check the changes I did
and let me know if it is good to make those changes.

Thanks,
Ravi

[dpdk-dev] [PATCH] doc: convert prog guide glossary to definition list

[Thomas Monjalon]

2015-04-10 16:39, John McNamara:
> Converted the Glossary table in the Programmer's Guide
> to a definition list to improve rendering.
> 
> Signed-off-by: John McNamara 

Applied, thanks

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Ananyev, Konstantin]

> -Original Message-
> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
> Sent: Tuesday, April 14, 2015 1:52 PM
> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin
> Cc: dev at dpdk.org
> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4
> 
> 
> 
> On 04/14/15 12:31, Thomas Monjalon wrote:
> > With GCC 4.4.7 from CentOS 6.5, the following errors arise:
> >
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
> > ?dev_info.driver_name?)
> >
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
> > ?dev_info.driver_name?)
> >
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
> > ?ixgbe_recv_pkts_lro_single_alloc?:
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
> > uninitialized in this function
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
> > uninitialized in this function
> 
> :D Looks like a gcc bug ;) Both are set and only after that (!!!) used
> under "!eop" condition.

Possibly, but we still need to make it build cleanly.
Konstantin

> 
> >
> > Fixes: 8eecb3295aed ("ixgbe: add LRO support")
> >
> > Signed-off-by: Thomas Monjalon 
> > ---
> >   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
> > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > index f1da9ec..a2b8631 100644
> > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
> > **rx_pkts, uint16_t nb_pkts,
> > bool eop;
> > struct ixgbe_rx_entry *rxe;
> > struct ixgbe_rsc_entry *rsc_entry;
> > -   struct ixgbe_rsc_entry *next_rsc_entry;
> > -   struct ixgbe_rx_entry *next_rxe;
> > +   struct ixgbe_rsc_entry *next_rsc_entry = NULL;
> > +   struct ixgbe_rx_entry *next_rxe = NULL;
> > struct rte_mbuf *first_seg;
> > struct rte_mbuf *rxm;
> > struct rte_mbuf *nmb;
> > @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > struct ixgbe_rx_queue *rxq;
> > struct ixgbe_hw *hw;
> > uint16_t len;
> > -   struct rte_eth_dev_info dev_info = { 0 };
> > +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> > struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
> > bool rsc_requested = false;
> >
> > @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
> >   {
> > struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
> > struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > -   struct rte_eth_dev_info dev_info = { 0 };
> > +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> > bool rsc_capable = false;
> > uint16_t i;
> > uint32_t rdrxctl;

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Ananyev, Konstantin]

> -Original Message-
> From: Vlad Zolotarov [mailto:vladz at cloudius-systems.com]
> Sent: Tuesday, April 14, 2015 1:49 PM
> To: Thomas Monjalon; Ananyev, Konstantin; Zhang, Helin
> Cc: dev at dpdk.org
> Subject: Re: [PATCH] ixgbe: fix build with gcc 4.4
> 
> 
> 
> On 04/14/15 12:31, Thomas Monjalon wrote:
> > With GCC 4.4.7 from CentOS 6.5, the following errors arise:
> >
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
> > ?dev_info.driver_name?)
> >
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
> > ?dev_info.driver_name?)
> >
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
> > ?ixgbe_recv_pkts_lro_single_alloc?:
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
> > uninitialized in this function
> > lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
> > uninitialized in this function
> >
> > Fixes: 8eecb3295aed ("ixgbe: add LRO support")
> >
> > Signed-off-by: Thomas Monjalon 
> > ---
> >   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
> > b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > index f1da9ec..a2b8631 100644
> > --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> > @@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
> > **rx_pkts, uint16_t nb_pkts,
> > bool eop;
> > struct ixgbe_rx_entry *rxe;
> > struct ixgbe_rsc_entry *rsc_entry;
> > -   struct ixgbe_rsc_entry *next_rsc_entry;
> > -   struct ixgbe_rx_entry *next_rxe;
> > +   struct ixgbe_rsc_entry *next_rsc_entry = NULL;
> > +   struct ixgbe_rx_entry *next_rxe = NULL;
> > struct rte_mbuf *first_seg;
> > struct rte_mbuf *rxm;
> > struct rte_mbuf *nmb;
> > @@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
> > struct ixgbe_rx_queue *rxq;
> > struct ixgbe_hw *hw;
> > uint16_t len;
> > -   struct rte_eth_dev_info dev_info = { 0 };
> > +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> > struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
> > bool rsc_requested = false;
> >
> > @@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
> >   {
> > struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
> > struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > -   struct rte_eth_dev_info dev_info = { 0 };
> > +   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
> 
> Hmmm... Unless I miss something this and one above would zero only a
> single field - "max_rx_queues"; and would leave the rest uninitialized.
> The original code intend to zero the whole struct. The alternative to
> the original lines could be usage of memset().

As I understand, in that case compiler had to set all non-explicitly 
initialised members to 0.
So I think we are ok here.

> 
> > bool rsc_capable = false;
> > uint16_t i;
> > uint32_t rdrxctl;

[dpdk-dev] [PATCH] ixgbe: fix build with gcc 4.4

[Thomas Monjalon]

With GCC 4.4.7 from CentOS 6.5, the following errors arise:

lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_dev_rx_queue_setup?:
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: missing initializer
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2509: error: (near initialization for 
?dev_info.driver_name?)

lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function ?ixgbe_set_rsc?:
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: missing initializer
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4072: error: (near initialization for 
?dev_info.driver_name?)

lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 
?ixgbe_recv_pkts_lro_single_alloc?:
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1479: error: ?next_rsc_entry? may be used 
uninitialized in this function
lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1480: error: ?next_rxe? may be used 
uninitialized in this function

Fixes: 8eecb3295aed ("ixgbe: add LRO support")

Signed-off-by: Thomas Monjalon 
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c 
b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
index f1da9ec..a2b8631 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
@@ -1476,8 +1476,8 @@ ixgbe_recv_pkts_lro(void *rx_queue, struct rte_mbuf 
**rx_pkts, uint16_t nb_pkts,
bool eop;
struct ixgbe_rx_entry *rxe;
struct ixgbe_rsc_entry *rsc_entry;
-   struct ixgbe_rsc_entry *next_rsc_entry;
-   struct ixgbe_rx_entry *next_rxe;
+   struct ixgbe_rsc_entry *next_rsc_entry = NULL;
+   struct ixgbe_rx_entry *next_rxe = NULL;
struct rte_mbuf *first_seg;
struct rte_mbuf *rxm;
struct rte_mbuf *nmb;
@@ -2506,7 +2506,7 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
struct ixgbe_rx_queue *rxq;
struct ixgbe_hw *hw;
uint16_t len;
-   struct rte_eth_dev_info dev_info = { 0 };
+   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
struct rte_eth_rxmode *dev_rx_mode = &dev->data->dev_conf.rxmode;
bool rsc_requested = false;

@@ -4069,7 +4069,7 @@ ixgbe_set_rsc(struct rte_eth_dev *dev)
 {
struct rte_eth_rxmode *rx_conf = &dev->data->dev_conf.rxmode;
struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-   struct rte_eth_dev_info dev_info = { 0 };
+   struct rte_eth_dev_info dev_info = { .max_rx_queues = 0 };
bool rsc_capable = false;
uint16_t i;
uint32_t rdrxctl;
-- 
2.2.2

[dpdk-dev] [PATCH] pci: make rte_pci_probe void

[Stephen Hemminger]

Since rte_pci_probe always returns 0 or exits via rte_exit()
there is no point in having it return a value.

Just make it void

Signed-off-by: Stephen Hemminger 
---
 lib/librte_eal/bsdapp/eal/eal.c | 3 +--
 lib/librte_eal/common/eal_common_pci.c  | 9 +++--
 lib/librte_eal/common/include/rte_pci.h | 6 +-
 lib/librte_eal/linuxapp/eal/eal.c   | 3 +--
 4 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal.c b/lib/librte_eal/bsdapp/eal/eal.c
index 871d5f4..9a9ada2 100644
--- a/lib/librte_eal/bsdapp/eal/eal.c
+++ b/lib/librte_eal/bsdapp/eal/eal.c
@@ -551,8 +551,7 @@ rte_eal_init(int argc, char **argv)
rte_eal_mp_wait_lcore();

/* Probe & Initialize PCI devices */
-   if (rte_eal_pci_probe())
-   rte_panic("Cannot probe PCI\n");
+   rte_eal_pci_probe();

return fctret;
 }
diff --git a/lib/librte_eal/common/eal_common_pci.c 
b/lib/librte_eal/common/eal_common_pci.c
index 808b87b..dbb4c92 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -223,13 +223,13 @@ err_return:
  * all registered drivers that have a matching entry in its id_table
  * for discovered devices.
  */
-int
+void
 rte_eal_pci_probe(void)
 {
struct rte_pci_device *dev = NULL;
struct rte_devargs *devargs;
int probe_all = 0;
-   int ret = 0;
+   int ret;

if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) == 0)
probe_all = 1;
@@ -252,12 +252,10 @@ rte_eal_pci_probe(void)
 " cannot be used\n", dev->addr.domain, 
dev->addr.bus,
 dev->addr.devid, dev->addr.function);
}
-
-   return 0;
 }

 /* dump one device */
-static int
+static void
 pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
 {
int i;
@@ -273,7 +271,6 @@ pci_dump_one_device(FILE *f, struct rte_pci_device *dev)
dev->mem_resource[i].phys_addr,
dev->mem_resource[i].len);
}
-   return 0;
 }

 /* dump devices on the bus */
diff --git a/lib/librte_eal/common/include/rte_pci.h 
b/lib/librte_eal/common/include/rte_pci.h
index 785852d..052d3da 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -327,12 +327,8 @@ int rte_eal_pci_scan(void);
  * Scan the content of the PCI bus, and call the probe() function for
  * all registered drivers that have a matching entry in its id_table
  * for discovered devices.
- *
- * @return
- *   - 0 on success.
- *   - Negative on error.
  */
-int rte_eal_pci_probe(void);
+void rte_eal_pci_probe(void);

 #ifdef RTE_LIBRTE_EAL_HOTPLUG
 /**
diff --git a/lib/librte_eal/linuxapp/eal/eal.c 
b/lib/librte_eal/linuxapp/eal/eal.c
index bd770cf..bd7ac62 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -842,8 +842,7 @@ rte_eal_init(int argc, char **argv)
rte_eal_mp_wait_lcore();

/* Probe & Initialize PCI devices */
-   if (rte_eal_pci_probe())
-   rte_panic("Cannot probe PCI\n");
+   rte_eal_pci_probe();

return fctret;
 }
-- 
2.1.4

[dpdk-dev] tools brainstorming

[Neil Horman]

On Tue, Apr 14, 2015 at 03:21:53PM +0100, Bruce Richardson wrote:
> On Wed, Apr 08, 2015 at 07:54:40PM +, Butler, Siobhan A wrote:
> > 
> > 
> > > -Original Message-
> > > From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > > Sent: Wednesday, April 8, 2015 5:16 PM
> > > To: Wiles, Keith; Butler, Siobhan A
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] tools brainstorming
> > > 
> > > 2015-04-08 15:53, Wiles, Keith:
> > > > One of the biggest problems with any style is helping the developer
> > > > maintain the style. Using some tool does help and I have used astyle
> > > > before, not bad code formatter. Here is a few that seem to be 
> > > > reasonable.
> > > >
> > > > http://astyle.sourceforge.net/
> > > >
> > > > http://uncrustify.sourceforge.net/
> > > >
> > > > http://sourceforge.net/projects/gcgreatcode/
> > > 
> > > I'm not sure it's a good idea to convert the codebase automatically.
> > > The coding style must be a reference for new patches and they must be
> > > automatically checked with a dedicated checkpatch tool.
> > > By forbidding patches which don't comply, the codebase will be naturally
> > > converted over time.
> > > 
> > > I didn't review this proposal yet.
> > > My first comment is that it's too long to read :) When a consensus is 
> > > done, it
> > > must be added with a patch with custom checkpatch addition.
> > Thanks Thomas, agreed it is a bit of a novel :)- I will refactor with the 
> > comments supplied so far and post a fresh version tomorrow.
> > Siobhan 
> > 
> 
> Just wondering here, are we looking to codify what the current predominant 
> coding
> style in DPDK *is* or what it *should be*? 
> 
> There has been some good discussion on a variety of areas, but if we focus on
> initially codifying what's there now, some issues become easier to resolve  -
> e.g. discussion of commenting style, since only C89 comments are allowed 
> right now.
> 

This is an excellent question.  I think the answer is we should make the style
what we want it to be. That said, when there is a significant discrepancy behind
what is wanted and what is, we need to stop and ask ourselves why that exists,
and what our reasoning is for wanting the change.

Neil

> /Bruce
> 

[dpdk-dev] [PATCH 1/5] bond: use existing enslaved device queues

[Eric Kinzie]

On Fri Apr 10 09:40:09 +0200 2015, Pawel Wodkowski wrote:
> On 2015-04-06 19:01, Eric Kinzie wrote:
> >If a device to be enslaved already has transmit and/or receive queues
> >allocated, use those and then create any additional queues that are
> >necessary.
> >
> >Signed-off-by: Eric Kinzie 
> >---
> >  lib/librte_pmd_bond/rte_eth_bond_pmd.c |8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/lib/librte_pmd_bond/rte_eth_bond_pmd.c 
> >b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> >index c937e6b..4fd7d97 100644
> >--- a/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> >+++ b/lib/librte_pmd_bond/rte_eth_bond_pmd.c
> >@@ -1318,7 +1318,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > }
> >
> > /* Setup Rx Queues */
> >-for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> >+/* Use existing queues, if any */
> >+for (q_id = slave_eth_dev->data->nb_rx_queues;
> >+ q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > bd_rx_q = (struct bond_rx_queue 
> > *)bonded_eth_dev->data->rx_queues[q_id];
> >
> > errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, 
> > q_id,
> >@@ -1334,7 +1336,9 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > }
> >
> > /* Setup Tx Queues */
> >-for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> >+/* Use existing queues, if any */
> >+for (q_id = slave_eth_dev->data->nb_tx_queues;
> >+ q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > bd_tx_q = (struct bond_tx_queue 
> > *)bonded_eth_dev->data->tx_queues[q_id];
> >
> > errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, 
> > q_id,
> >
> 
> Why you want to do that?
> 
> As far as I am aware (but Declan Doherty should speak here to) purpose
> of this part of code is to have configuration of queues in slaves
> consistent with bd_rx_q/bd_tx_q. If you skip reconfiguration of queues
> that are already configured in port you can have them configured
> in different way after enslaving.
> 
> So again: what is the purpose of doing so?
> 
> -- 
> Pawel

Pawel,

I generally test things I've just built using virtio devices and calling
rte_eth_tx_queue_setup() more than once for a given queue id fails.
However, it seems that most PMDs allow re-allocating device queues while
virtio does not (xenvirt also seems to lack this functionality), so I
don't think my approach here is right.  I'll remove this patch when I
send the next version of this series.

Thanks,

Eric

[dpdk-dev] [PATCH 2/2] pci: rearrange logic from compare loop

[Stephen Hemminger]

There is no need to initialize a variable that is only used as a loop
variable.



On Tue, Apr 14, 2015 at 2:30 AM, Qiu, Michael  wrote:

> On 4/14/2015 6:11 AM, Stephen Hemminger wrote:
> > Do some cleanup of pci scan loop.
> >   * check errors first
> >   * don't initialize variables where not necessary
>
> Why? It should be better to initialize variables when define it.
>
> Thanks,
> Michael
> >   * cuddle else (follow existing style)
> >   * chop off conditional after return
> >
> > Signed-off-by: Stephen Hemminger 
> >
> > ---
> >  lib/librte_eal/linuxapp/eal/eal_pci.c | 24 
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > index c98a778..d96b1c4 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> > @@ -337,6 +337,12 @@ pci_scan_one(const char *dirname, uint16_t domain,
> uint8_t bus,
> >   /* parse driver */
> >   snprintf(filename, sizeof(filename), "%s/driver", dirname);
> >   ret = pci_get_kernel_driver_by_path(filename, driver);
> > + if (ret < 0) {
> > + RTE_LOG(ERR, EAL, "Fail to get kernel driver\n");
> > + free(dev);
> > + return -1;
> > + }
> > +
> >   if (!ret) {
> >   if (!strcmp(driver, "vfio-pci"))
> >   dev->kdrv = RTE_KDRV_VFIO;
> > @@ -346,37 +352,31 @@ pci_scan_one(const char *dirname, uint16_t domain,
> uint8_t bus,
> >   dev->kdrv = RTE_KDRV_UIO_GENERIC;
> >   else
> >   dev->kdrv = RTE_KDRV_UNKNOWN;
> > - } else if (ret < 0) {
> > - RTE_LOG(ERR, EAL, "Fail to get kernel driver\n");
> > - free(dev);
> > - return -1;
> >   } else
> >   dev->kdrv = RTE_KDRV_UNKNOWN;
> >
> >   /* device is valid, add in list (sorted) */
> >   if (TAILQ_EMPTY(&pci_device_list)) {
> >   TAILQ_INSERT_TAIL(&pci_device_list, dev, next);
> > - }
> > - else {
> > - struct rte_pci_device *dev2 = NULL;
> > + } else {
> > + struct rte_pci_device *dev2;
> >   int ret;
> >
> >   TAILQ_FOREACH(dev2, &pci_device_list, next) {
> >   ret = rte_eal_compare_pci_addr(&dev->addr,
> &dev2->addr);
> >   if (ret > 0)
> >   continue;
> > - else if (ret < 0) {
> > +
> > + if (ret < 0) {
> >   TAILQ_INSERT_BEFORE(dev2, dev, next);
> > - return 0;
> >   } else { /* already registered */
> >   dev2->kdrv = dev->kdrv;
> >   dev2->max_vfs = dev->max_vfs;
> > - memmove(dev2->mem_resource,
> > - dev->mem_resource,
> > + memmove(dev2->mem_resource,
> dev->mem_resource,
> >   sizeof(dev->mem_resource));
> >   free(dev);
> > - return 0;
> >   }
> > + return 0;
> >   }
> >   TAILQ_INSERT_TAIL(&pci_device_list, dev, next);
> >   }
>
>

[dpdk-dev] [PATCH 2/2] pci: rearrange logic from compare loop

[Qiu, Michael]

On 4/14/2015 6:11 AM, Stephen Hemminger wrote:
> Do some cleanup of pci scan loop.
>   * check errors first
>   * don't initialize variables where not necessary

Why? It should be better to initialize variables when define it.

Thanks,
Michael
>   * cuddle else (follow existing style)
>   * chop off conditional after return
>
> Signed-off-by: Stephen Hemminger 
>
> ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c | 24 
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c 
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index c98a778..d96b1c4 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -337,6 +337,12 @@ pci_scan_one(const char *dirname, uint16_t domain, 
> uint8_t bus,
>   /* parse driver */
>   snprintf(filename, sizeof(filename), "%s/driver", dirname);
>   ret = pci_get_kernel_driver_by_path(filename, driver);
> + if (ret < 0) {
> + RTE_LOG(ERR, EAL, "Fail to get kernel driver\n");
> + free(dev);
> + return -1;
> + }
> +
>   if (!ret) {
>   if (!strcmp(driver, "vfio-pci"))
>   dev->kdrv = RTE_KDRV_VFIO;
> @@ -346,37 +352,31 @@ pci_scan_one(const char *dirname, uint16_t domain, 
> uint8_t bus,
>   dev->kdrv = RTE_KDRV_UIO_GENERIC;
>   else
>   dev->kdrv = RTE_KDRV_UNKNOWN;
> - } else if (ret < 0) {
> - RTE_LOG(ERR, EAL, "Fail to get kernel driver\n");
> - free(dev);
> - return -1;
>   } else
>   dev->kdrv = RTE_KDRV_UNKNOWN;
>  
>   /* device is valid, add in list (sorted) */
>   if (TAILQ_EMPTY(&pci_device_list)) {
>   TAILQ_INSERT_TAIL(&pci_device_list, dev, next);
> - }
> - else {
> - struct rte_pci_device *dev2 = NULL;
> + } else {
> + struct rte_pci_device *dev2;
>   int ret;
>  
>   TAILQ_FOREACH(dev2, &pci_device_list, next) {
>   ret = rte_eal_compare_pci_addr(&dev->addr, &dev2->addr);
>   if (ret > 0)
>   continue;
> - else if (ret < 0) {
> +
> + if (ret < 0) {
>   TAILQ_INSERT_BEFORE(dev2, dev, next);
> - return 0;
>   } else { /* already registered */
>   dev2->kdrv = dev->kdrv;
>   dev2->max_vfs = dev->max_vfs;
> - memmove(dev2->mem_resource,
> - dev->mem_resource,
> + memmove(dev2->mem_resource, dev->mem_resource,
>   sizeof(dev->mem_resource));
>   free(dev);
> - return 0;
>   }
> + return 0;
>   }
>   TAILQ_INSERT_TAIL(&pci_device_list, dev, next);
>   }

[dpdk-dev] [PATCH 1/2] pci: cleanup whitespace

[Qiu, Michael]

On 4/14/2015 6:11 AM, Stephen Hemminger wrote:
> Fix whitespace errors reported by checkpatch, including
> missing space around operators and places where tab should
> be used instead of space.
>
> Signed-off-by: Stephen Hemminger 

Acked-by: Michael Qiu 

> ---
>  lib/librte_eal/linuxapp/eal/eal_pci.c | 17 -
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c 
> b/lib/librte_eal/linuxapp/eal/eal_pci.c
> index 9cb0ffd..c98a778 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
> @@ -68,22 +68,22 @@ pci_unbind_kernel_driver(struct rte_pci_device *dev)
>  
>   /* open /sys/bus/pci/devices/:BB:CC.D/driver */
>   snprintf(filename, sizeof(filename),
> -  SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/driver/unbind",
> -  loc->domain, loc->bus, loc->devid, loc->function);
> +  SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/driver/unbind",
> +  loc->domain, loc->bus, loc->devid, loc->function);
>  
>   f = fopen(filename, "w");
>   if (f == NULL) /* device was not bound */
>   return 0;
>  
>   n = snprintf(buf, sizeof(buf), PCI_PRI_FMT "\n",
> -  loc->domain, loc->bus, loc->devid, loc->function);
> +  loc->domain, loc->bus, loc->devid, loc->function);
>   if ((n < 0) || (n >= (int)sizeof(buf))) {
>   RTE_LOG(ERR, EAL, "%s(): snprintf failed\n", __func__);
>   goto error;
>   }
>   if (fwrite(buf, n, 1, f) == 0) {
>   RTE_LOG(ERR, EAL, "%s(): could not write to %s\n", __func__,
> - filename);
> + filename);
>   goto error;
>   }
>  
> @@ -205,8 +205,7 @@ pci_parse_sysfs_resource(const char *filename, struct 
> rte_pci_device *dev)
>   return -1;
>   }
>  
> - for (i = 0; i -
> + for (i = 0; i < PCI_MAX_RESOURCE; i++) {
>   if (fgets(buf, sizeof(buf), f) == NULL) {
>   RTE_LOG(ERR, EAL,
>   "%s(): cannot read resource\n", __func__);
> @@ -402,8 +401,8 @@ parse_pci_addr_format(const char *buf, int bufsize, 
> uint16_t *domain,
>   };
>   char *str[PCI_FMT_NVAL]; /* last element-separator is "." not 
> ":" */
>   } splitaddr;
> -
>   char *buf_copy = strndup(buf, bufsize);
> +
>   if (buf_copy == NULL)
>   return -1;
>  
> @@ -411,7 +410,7 @@ parse_pci_addr_format(const char *buf, int bufsize, 
> uint16_t *domain,
>   != PCI_FMT_NVAL - 1)
>   goto error;
>   /* final split is on '.' between devid and function */
> - splitaddr.function = strchr(splitaddr.devid,'.');
> + splitaddr.function = strchr(splitaddr.devid, '.');
>   if (splitaddr.function == NULL)
>   goto error;
>   *splitaddr.function++ = '\0';
> @@ -671,7 +670,7 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, 
> struct rte_pci_device *d
>   if (ret != 0)
>   return ret;
>   } else if (dr->drv_flags & RTE_PCI_DRV_FORCE_UNBIND &&
> -rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +rte_eal_process_type() == RTE_PROC_PRIMARY) {
>   /* unbind current driver */
>   if (pci_unbind_kernel_driver(dev) < 0)
>   return -1;

[dpdk-dev] [PATCH 1/2] ixgbe: silence noisy log messages

[Zhang, Helin]

> -Original Message-
> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
> Sent: Friday, April 10, 2015 11:18 PM
> To: Zhang, Helin; Ananyev, Konstantin
> Cc: dev at dpdk.org; Stephen Hemminger
> Subject: [PATCH 1/2] ixgbe: silence noisy log messages
> 
> The ixgbe driver likes to be far to chatty in the system log which is good 
> for the
> original developer but not good for a production product.
> 
> Reduce the log spam by doing:
>  * All the normal messages should be changed from INFO to DEBUG.
>  * The log messages should be done with RTE_LOG so that they can be
>compiled out if RTE_LOG_LEVEL is set.
>  * The link state print routine prints more than is necessary
>PCI information is already known (earlier in log) and has
>no purpose here.
> 
> Signed-off-by: Stephen Hemminger 
> ---
>  lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 41 
> -
>  lib/librte_pmd_ixgbe/ixgbe_fdir.c   |  2 +-
>  lib/librte_pmd_ixgbe/ixgbe_logs.h   |  3 +--
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c   | 14 ++---
>  4 files changed, 27 insertions(+), 33 deletions(-)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> index 5caee22..adc0fb9 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
> @@ -569,8 +569,8 @@ ixgbe_dev_queue_stats_mapping_set(struct
> rte_eth_dev *eth_dev,
>   (hw->mac.type != ixgbe_mac_X550EM_x))
>   return -ENOSYS;
> 
> - PMD_INIT_LOG(INFO, "Setting port %d, %s queue_id %d to stat
> index %d",
> -  (int)(eth_dev->data->port_id), is_rx ? "RX" : "TX",
> + PMD_INIT_LOG(DEBUG, "Setting port %u, %s queue_id %d to stat
> index %d",
> +  eth_dev->data->port_id, is_rx ? "RX" : "TX",
>queue_id, stat_idx);
> 
>   n = (uint8_t)(queue_id / NB_QMAP_FIELDS_PER_QSM_REG); @@ -595,20
> +595,20 @@ ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev
> *eth_dev,
>   else
>   stat_mappings->rqsmr[n] |= qsmr_mask;
> 
> - PMD_INIT_LOG(INFO, "Set port %d, %s queue_id %d to stat index %d",
> -  (int)(eth_dev->data->port_id), is_rx ? "RX" : "TX",
> + PMD_INIT_LOG(DEBUG, "Set port %u, %s queue_id %d to stat index %d",
> +  eth_dev->data->port_id, is_rx ? "RX" : "TX",
>queue_id, stat_idx);
> - PMD_INIT_LOG(INFO, "%s[%d] = 0x%08x", is_rx ? "RQSMR" : "TQSM", n,
> + PMD_INIT_LOG(DEBUG, "%s[%d] = 0x%08x", is_rx ? "RQSMR" : "TQSM",
> n,
>is_rx ? stat_mappings->rqsmr[n] : stat_mappings->tqsm[n]);
> 
>   /* Now write the mapping in the appropriate register */
>   if (is_rx) {
> - PMD_INIT_LOG(INFO, "Write 0x%x to RX IXGBE stat mapping
> reg:%d",
> + PMD_INIT_LOG(DEBUG, "Write 0x%x to RX IXGBE stat mapping
> reg:%d",
>stat_mappings->rqsmr[n], n);
>   IXGBE_WRITE_REG(hw, IXGBE_RQSMR(n),
> stat_mappings->rqsmr[n]);
>   }
>   else {
> - PMD_INIT_LOG(INFO, "Write 0x%x to TX IXGBE stat mapping
> reg:%d",
> + PMD_INIT_LOG(DEBUG, "Write 0x%x to TX IXGBE stat mapping
> reg:%d",
>stat_mappings->tqsm[n], n);
>   IXGBE_WRITE_REG(hw, IXGBE_TQSM(n), stat_mappings->tqsm[n]);
>   }
> @@ -752,7 +752,7 @@ eth_ixgbe_dev_init(struct rte_eth_dev *eth_dev)
>   ixgbe_set_tx_function(eth_dev, txq);
>   } else {
>   /* Use default TX function if we get here */
> - PMD_INIT_LOG(INFO, "No TX queues configured yet. "
> + PMD_INIT_LOG(DEBUG, "No TX queues configured yet. "
>  "Using default TX function.");
>   }
> 
> @@ -1249,7 +1249,7 @@ ixgbe_vlan_hw_strip_disable(struct rte_eth_dev
> *dev, uint16_t queue)
> 
>   if (hw->mac.type == ixgbe_mac_82598EB) {
>   /* No queue level support */
> - PMD_INIT_LOG(INFO, "82598EB not support queue level hw strip");
> + PMD_INIT_LOG(NOTICE, "82598EB not support queue level hw
> strip");
>   return;
>   }
>   else {
> @@ -1273,7 +1273,7 @@ ixgbe_vlan_hw_strip_enable(struct rte_eth_dev
> *dev, uint16_t queue)
> 
>   if (hw->mac.type == ixgbe_mac_82598EB) {
>   /* No queue level supported */
> - PMD_INIT_LOG(INFO, "82598EB not support queue level hw strip");
> + PMD_INIT_LOG(NOTICE, "82598EB not support queue level hw
> strip");
>   return;
>   }
>   else {
> @@ -2265,7 +2265,7 @@ ixgbe_dev_interrupt_get_status(struct rte_eth_dev
> *dev)
> 
>   /* read-on-clear nic registers here */
>   eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
> - PMD_DRV_LOG(INFO, "eicr %x", eicr);
> + PMD_DRV_LOG(DEBUG, "eicr %x", eicr);
> 
>   intr->flags = 0;
>   if (eicr & IXGBE_EICR_LSC) {
> @@ -2

[dpdk-dev] [PATCH] Fixed spam from kni_allocate_mbufs() when no mbufs are free. If mbufs exhausted, 'out of memory' message logged at EXTREMELY high rates. Now logs no more than once per 10 mins

[Jay Rolette]

Hi Stephen,

Thanks for the feedback. Comments and questions inline below.

Jay

On Mon, Apr 13, 2015 at 8:09 PM, Stephen Hemminger <
stephen at networkplumber.org> wrote:

> On Wed, 17 Dec 2014 07:57:02 -0600
> Jay Rolette  wrote:
>
> > Signed-off-by: Jay Rolette 
> > ---
> >  lib/librte_kni/rte_kni.c | 21 -
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> > index fdb7509..f89319c 100644
> > --- a/lib/librte_kni/rte_kni.c
> > +++ b/lib/librte_kni/rte_kni.c
> > @@ -40,6 +40,7 @@
> >  #include 
> >  #include 
> >
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -61,6 +62,9 @@
> >
> >  #define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)
> >
> > +// Configure how often we log "out of memory" messages (in seconds)
> > +#define KNI_SPAM_SUPPRESSION_PERIOD 60*10
> > +
> >  /**
> >   * KNI context
> >   */
> > @@ -592,6 +596,10 @@ kni_free_mbufs(struct rte_kni *kni)
> >  static void
> >  kni_allocate_mbufs(struct rte_kni *kni)
> >  {
> > + static uint64_t no_mbufs = 0;
> > + static uint64_t spam_filter = 0;
> > + static uint64_t delayPeriod = 0;
> > +
> >   int i, ret;
> >   struct rte_mbuf *pkts[MAX_MBUF_BURST_NUM];
> >
> > @@ -620,7 +628,18 @@ kni_allocate_mbufs(struct rte_kni *kni)
> >   pkts[i] = rte_pktmbuf_alloc(kni->pktmbuf_pool);
> >   if (unlikely(pkts[i] == NULL)) {
> >   /* Out of memory */
> > - RTE_LOG(ERR, KNI, "Out of memory\n");
> > + no_mbufs++;
> > +
> > + // Memory leak or need to tune? Regardless, if we
> get here once,
> > + // we will get here a *lot*. Don't spam the logs!
> > + now = rte_get_tsc_cycles();
> > + if (!delayPeriod)
> > + delayPeriod = rte_get_tsc_hz() *
> KNI_SPAM_SUPPRESSION_PERIOD;
> > +
> > + if (!spam_filter || (now - spam_filter) >
> delayPeriod) {
> > + RTE_LOG(ERR, KNI, "No mbufs available
> (%llu)\n", (unsigned long long)no_mbufs);
> > + spam_filter = now;
> > + }
> >   break;
> >   }
> >   }
>
> I agree whole completely with the intent of this.
> But just remove the log message completely. It doesn't
> help at all, use a statistic instead.
>

I'm fine with removing the log message completely. Can you point me to
where DPDK keeps stats generally? Stats like this are only useful if they
are accessible from some sort of monitoring process. There aren't any stats
in struct rte_kni right now.

If you want to do ratelimiting, then it is better to create
> a common function (see net_ratelimit() in Linux kernel) to
> have all code use the same method, rather than reinventing private
> code to do it.
>

I'll remove it.


> Minor style complaints:
>   * don't use camelCase, it goes against the style of the rest of the code.
>

ok


>   * don't use C++ style comments.
>

I didn't. I used C99 style comments :)


>   * always use rte_cycles() not TSC for things like this.
>

I don't see rte_cycles() defined anywhere. Did you mean some other function?

Please resubmit removing the log message and adding a statistic.
>

[dpdk-dev] [PATCH] doc: fix vhost guide

[Ouyang, Changchun]

From: Igor Ryzhov [mailto:iryz...@nfware.com]
Sent: Monday, April 13, 2015 3:14 PM
To: Ouyang, Changchun
Cc: dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH] doc: fix vhost guide

Sorry, I used wrong email address to reply from. This one is correct.

On Mon, Apr 13, 2015 at 10:11 AM, Igor Ryzhov mailto:iryzhov at arccn.ru>> wrote:
Hello, Changchun.

Previous paragraph says ?To enable vhost, turn on vhost library in the 
configure file config/common_linuxapp?, but string in a code-block is 
?CONFIG_RTE_LIBRTE_VHOST=n?. I thought that idea is to use the default string 
from the config file that user have to change, not already changed string. So I 
used the same style.

Regards,
Igor

13 ???. 2015 ?., ? 7:52, Ouyang, Changchun mailto:changchun.ouyang at intel.com>> ???(?):

Hi Igor,

Good catch, comments as below.


-Original Message-
From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Igor Ryzhov
Sent: Thursday, April 9, 2015 12:31 AM
To: dev at dpdk.org
Cc: Igor Ryzhov
Subject: [dpdk-dev] [PATCH] doc: fix vhost guide

Guide says that a configure parameter to choose between vhost cuse and
vhost user will be introduced in the future, but it?s already added by commit
28a1ccca41bf.

Signed-off-by: Igor Ryzhov mailto:iryzhov at nfware.com>>

Acked-by: Changchun Ouyang 

[dpdk-dev] [PATCH v3] Restore support for virtio on FreeBSD

[Ouyang, Changchun]

Hi 

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, April 13, 2015 8:55 PM
> To: Raz Amir
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] Restore support for virtio on FreeBSD
> 
> Please provide more information in the commit message.
> We need to know what was the problem (crash) in the git history.
> Then when doing git blame, we'll have the full explanation.
> 
> 2015-04-13 15:19, Raz Amir:
> > Fixes: 8a312224bcde ("eal/bsd: fix fd leak")
> >
> > Signed-off-by: Raz Amir 
> > ---
> >  lib/librte_eal/bsdapp/eal/eal.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/eal.c
> > b/lib/librte_eal/bsdapp/eal/eal.c index 871d5f4..e20f915 100644
> > --- a/lib/librte_eal/bsdapp/eal/eal.c
> > +++ b/lib/librte_eal/bsdapp/eal/eal.c
> > @@ -426,7 +426,7 @@ rte_eal_iopl_init(void)
> > fd = open("/dev/io", O_RDWR);
> > if (fd < 0)
> > return -1;
> > -   close(fd);
> > +   /* keep fd open for iopl */

Copy and paste my comment into this new patch:
Would you pls think about this solution?
Declare a static var to keep the fd which is opened for freebsd;
Then define a deinit function for virtio device, Inside the deinit function, 
close the fd which was opened in init stage.
Done.

thanks
Changchun

[dpdk-dev] Crash related to virtio NICs in DPDK 2.0.0 on Freebsd 10.1 VM

[Ouyang, Changchun]

Hi Raz,

Thanks for identifying this issue.
A comments below.

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Raz Amir
> Sent: Monday, April 13, 2015 7:54 PM
> To: 'Thomas Monjalon'
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] Crash related to virtio NICs in DPDK 2.0.0 on Freebsd
> 10.1 VM
> 
> Thanks. I will submit a patch

When you submit another patch,
Would you pls think about this solution?
Declare a static var to keep the fd which is opened for freebsd 
Then define a deinit function for virtio device,
Inside the deinit function, close the fd which was opened in init stage.
Done.

thanks 
Changchun

> 
> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: 13 April 2015 13:46
> To: Raz Amir
> Cc: dev at dpdk.org; david.marchand at 6wind.com
> Subject: Re: Crash related to virtio NICs in DPDK 2.0.0 on Freebsd 10.1 VM
> 
> 2015-04-08 18:53, Raz Amir:
> > The issues happens also in dpdk 1.8.0, and related to patch
> > http://dpdk.org/dev/patchwork/patch/239/
> >
> > Adding Thomas and David to the thread and I will appreciate your input.
> >
> > The patch comes to solve a file descriptor leak in the bsdapp version
> > of rte_eal_iopl_init after opening the /dev/io device.
> >
> > Seems like this isn't a file descriptor leak, and it should remain
> > open - as I wrote below, I am using virtio.
> 
> Thanks for the bug report.
> It seems there was no validation for FreeBSD with virtio.
> 
> > After removing it and testing the crash was resolved.
> >
> > Any objection for removing the close(fd) that was added at dpdk 1.8.0?
> 
> No, there was a doubt because the man page was not clear.
>   http://www.freebsd.org/cgi/man.cgi?query=io&sektion=4
> 
> In case you submit a patch, please add this line:
> Fixes: 8a312224bcde ("eal/bsd: fix fd leak")
> 
> > Are there scenarios that might be impacted by removing it?
> 
> I don't think so.
> 
> Thanks