[dpdk-dev] [PATCH 00/10] eal: rte_tailq api cleanup

[Thomas Monjalon]

2015-03-04 22:50, David Marchand:
> 143 files changed, 310 insertions(+), 685 deletions(-)

Great job.
As it removes more lines than it adds, it's probably a good cleanup patchset ;)

-- 
Thomas
When there is no more things to remove, it's becoming perfect.

[dpdk-dev] KNI with multiple kthreads per port

[JP M.]

On Wed, Mar 4, 2015 at 9:40 PM, Zhang, Helin  wrote:
>>  * 32-bit (yes, KNI works fine, after a few tweaks hugepage init strategy)
> Interesting! How did you get it works?

In a nutshell: The current (circa r1.8.0) approach does mmap starting
from the bottom of the address space, then does a second round of
mmap, then unmaps the first round. Result is (AFAICT) unnecessarily
wasting much of the address space. I'll try to get my patch cleaned up
and posted in the next several days.

> What do you mean "vEth0_1 cannot be configured"?

The primary KNI device, vEth0_0, has (I forget which) ops defined, but
any secndary KNI devices do not. That said, I now see that once the
latter are brought up, I can set their MAC address. Which turns out to
be important. (see below)

>> Should I be bonding vEth0_0 and vEth0_1?  Because I tried doing so (via 
>> sysfs);
>> however, attempts to add either as slaves to bond0 were ignored.
>
> What do you mean bonding here? Basically KNI has no relationship to bonding.

All the same, I figured out what I was doing wrong (user-error on my
part, I think) with regards to bonding (EtherChannel) and am now able
to get multiple vnics to enslave. The catch was that the secondary KNI
devices, after being enslaved, are assigned a random MAC. After doing
so, it is necessary to then manually set their MAC to that of the
primary KNI device.  Thereafter, Rx/Tx are load-balanced as expected.
That assignment of a random MAC to the secondary KNI devices is the
hang-up; not clear whether there's a deliberate reason for that
happening.

 ~jp

[dpdk-dev] [PATCH 10/10] eal: no need for E_RTE_NO_TAILQ anymore

[David Marchand]

There is no remaining reference to this errno.

Signed-off-by: David Marchand 
---
 app/test/test_errno.c |2 +-
 lib/librte_eal/common/eal_common_errno.c  |2 --
 lib/librte_eal/common/include/rte_errno.h |1 -
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/test/test_errno.c b/app/test/test_errno.c
index c903b19..f221eac 100644
--- a/app/test/test_errno.c
+++ b/app/test/test_errno.c
@@ -58,7 +58,7 @@ test_errno(void)
/* use a small selection of standard errors for testing */
int std_errs[] = {EAGAIN, EBADF, EACCES, EINTR, EINVAL};
/* test ALL registered RTE error codes for overlap */
-   int rte_errs[] = {E_RTE_SECONDARY, E_RTE_NO_CONFIG, E_RTE_NO_TAILQ};
+   int rte_errs[] = {E_RTE_SECONDARY, E_RTE_NO_CONFIG};
unsigned i;

rte_errno = 0;
diff --git a/lib/librte_eal/common/eal_common_errno.c 
b/lib/librte_eal/common/eal_common_errno.c
index 259f895..de48d8e 100644
--- a/lib/librte_eal/common/eal_common_errno.c
+++ b/lib/librte_eal/common/eal_common_errno.c
@@ -64,8 +64,6 @@ rte_strerror(int errnum)
return "Invalid call in secondary process";
case E_RTE_NO_CONFIG:
return "Missing rte_config structure";
-   case E_RTE_NO_TAILQ:
-   return "No TAILQ initialised";
default:
strerror_r(errnum, RTE_PER_LCORE(retval), RETVAL_SZ);
}
diff --git a/lib/librte_eal/common/include/rte_errno.h 
b/lib/librte_eal/common/include/rte_errno.h
index 45910cd..2e5cc45 100644
--- a/lib/librte_eal/common/include/rte_errno.h
+++ b/lib/librte_eal/common/include/rte_errno.h
@@ -84,7 +84,6 @@ enum {

E_RTE_SECONDARY, /**< Operation not allowed in secondary processes */
E_RTE_NO_CONFIG, /**< Missing rte_config */
-   E_RTE_NO_TAILQ,  /**< Uninitialised TAILQ */

RTE_MAX_ERRNO/**< Max RTE error number */
 };
-- 
1.7.10.4

[dpdk-dev] [PATCH 09/10] tailq: remove static slots

[David Marchand]

No static entry remaining, the rte_tailq api is for "internal use" only, get rid
of the static slots.

Signed-off-by: David Marchand 
---
 lib/librte_eal/bsdapp/eal/rte_eal_version.map |1 -
 lib/librte_eal/common/Makefile|2 +-
 lib/librte_eal/common/eal_common_tailqs.c |   47 +-
 lib/librte_eal/common/include/rte_eal_memconfig.h |9 ---
 lib/librte_eal/common/include/rte_tailq.h |   34 --
 lib/librte_eal/common/include/rte_tailq_elem.h|   70 -
 lib/librte_eal/linuxapp/eal/rte_eal_version.map   |1 -
 7 files changed, 3 insertions(+), 161 deletions(-)
 delete mode 100644 lib/librte_eal/common/include/rte_tailq_elem.h

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map 
b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index e42ea74..67b6a6c 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -44,7 +44,6 @@ DPDK_2.0 {
rte_eal_process_type;
rte_eal_remote_launch;
rte_eal_tailq_lookup;
-   rte_eal_tailq_lookup_by_idx;
rte_eal_tailq_register;
rte_eal_wait_lcore;
rte_exit;
diff --git a/lib/librte_eal/common/Makefile b/lib/librte_eal/common/Makefile
index 52c1a5f..cf961a7 100644
--- a/lib/librte_eal/common/Makefile
+++ b/lib/librte_eal/common/Makefile
@@ -36,7 +36,7 @@ INC += rte_debug.h rte_eal.h rte_errno.h rte_launch.h 
rte_lcore.h
 INC += rte_log.h rte_memory.h rte_memzone.h rte_pci.h
 INC += rte_pci_dev_ids.h rte_per_lcore.h rte_random.h
 INC += rte_rwlock.h rte_tailq.h rte_interrupts.h rte_alarm.h
-INC += rte_string_fns.h rte_version.h rte_tailq_elem.h
+INC += rte_string_fns.h rte_version.h
 INC += rte_eal_memconfig.h rte_malloc_heap.h
 INC += rte_hexdump.h rte_devargs.h rte_dev.h
 INC += rte_common_vect.h
diff --git a/lib/librte_eal/common/eal_common_tailqs.c 
b/lib/librte_eal/common/eal_common_tailqs.c
index 3c4e70d..d9551cd 100644
--- a/lib/librte_eal/common/eal_common_tailqs.c
+++ b/lib/librte_eal/common/eal_common_tailqs.c
@@ -55,14 +55,6 @@

 #include "eal_private.h"

-/**
- * Name of tailq_head
- */
-const char* rte_tailq_names[RTE_MAX_TAILQ] = {
-#define rte_tailq_elem(idx, name) name,
-#include 
-};
-
 TAILQ_HEAD(rte_tailq_elem_head, rte_tailq_elem);
 /* local tailq list */
 static struct rte_tailq_elem_head rte_tailq_elem_head =
@@ -81,11 +73,6 @@ rte_eal_tailq_lookup(const char *name)
return NULL;

for (i = 0; i < RTE_MAX_TAILQ; i++) {
-   if (i < RTE_TAILQ_NUM &&
-   !strncmp(name, rte_tailq_names[i], RTE_TAILQ_NAMESIZE-1))
-   return >tailq_head[i];
-
-   /* if past static entries, look at shared mem for names */
if (!strncmp(name, mcfg->tailq_head[i].name,
 RTE_TAILQ_NAMESIZE-1))
return >tailq_head[i];
@@ -94,19 +81,6 @@ rte_eal_tailq_lookup(const char *name)
return NULL;
 }

-inline struct rte_tailq_head *
-rte_eal_tailq_lookup_by_idx(const unsigned tailq_idx)
-{
-   struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
-
-   if (tailq_idx >= RTE_MAX_TAILQ) {
-   RTE_LOG(ERR, EAL, "%s(): No more room in config\n", __func__);
-   return NULL;
-   }
-
-   return >tailq_head[tailq_idx];
-}
-
 void
 rte_dump_tailq(FILE *f)
 {
@@ -119,15 +93,9 @@ rte_dump_tailq(FILE *f)
for (i = 0; i < RTE_MAX_TAILQ; i++) {
const struct rte_tailq_head *tailq = >tailq_head[i];
const struct rte_tailq_entry_head *head = >tailq_head;
-   const char *name = "nil";
-
-   if (rte_tailq_names[i])
-   name = rte_tailq_names[i];
-   else if (tailq->name)
-   name = tailq->name;

fprintf(f, "Tailq %u: qname:<%s>, tqh_first:%p, tqh_last:%p\n",
-   i, name, head->tqh_first, head->tqh_last);
+   i, tailq->name, head->tqh_first, head->tqh_last);
}
rte_rwlock_read_unlock(>qlock);
 }
@@ -209,20 +177,9 @@ error:
 int
 rte_eal_tailqs_init(void)
 {
-   unsigned i;
-   struct rte_mem_config *mcfg = NULL;
struct rte_tailq_elem *t;

-   RTE_BUILD_BUG_ON(RTE_MAX_TAILQ < RTE_TAILQ_NUM);
-
-   if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
-   mcfg = rte_eal_get_configuration()->mem_config;
-   for (i = 0; i < RTE_TAILQ_NUM; i++)
-   TAILQ_INIT(>tailq_head[i].tailq_head);
-   }
-
-   /* mark those static entries as already taken */
-   rte_tailqs_count = RTE_TAILQ_NUM;
+   rte_tailqs_count = 0;

TAILQ_FOREACH(t, _tailq_elem_head, next) {
/* second part of register job for "early" tailqs, see
diff --git a/lib/librte_eal/common/include/rte_eal_memconfig.h 
b/lib/librte_eal/common/include/rte_eal_memconfig.h
index 

[dpdk-dev] [PATCH 08/10] tailq: move to dynamic tailq

[David Marchand]

Use dynamic tailq rather than static entries.

Signed-off-by: David Marchand 
---
 lib/librte_acl/rte_acl.c   |   33 +++
 lib/librte_acl/rte_acl.h   |1 -
 lib/librte_distributor/rte_distributor.c   |   16 +--
 lib/librte_eal/bsdapp/eal/eal_pci.c|7 -
 lib/librte_eal/common/include/rte_tailq_elem.h |   20 --
 lib/librte_eal/linuxapp/eal/eal_ivshmem.c  |5 ++--
 lib/librte_eal/linuxapp/eal/eal_pci.c  |8 --
 lib/librte_hash/rte_fbk_hash.c |   32 --
 lib/librte_hash/rte_fbk_hash.h |1 -
 lib/librte_hash/rte_hash.c |   27 ++-
 lib/librte_hash/rte_hash.h |1 -
 lib/librte_lpm/rte_lpm.c   |   34 +---
 lib/librte_lpm/rte_lpm.h   |1 -
 lib/librte_lpm/rte_lpm6.c  |   26 ++
 lib/librte_lpm/rte_lpm6.h  |1 -
 lib/librte_mempool/rte_mempool.c   |   32 +++---
 lib/librte_mempool/rte_mempool.h   |3 ---
 lib/librte_reorder/rte_reorder.c   |   26 ++
 lib/librte_reorder/rte_reorder.h   |1 -
 lib/librte_ring/rte_ring.c |   27 +++
 lib/librte_ring/rte_ring.h |3 ++-
 21 files changed, 99 insertions(+), 206 deletions(-)

diff --git a/lib/librte_acl/rte_acl.c b/lib/librte_acl/rte_acl.c
index 7d10301..b6ddeeb 100644
--- a/lib/librte_acl/rte_acl.c
+++ b/lib/librte_acl/rte_acl.c
@@ -38,6 +38,11 @@

 TAILQ_HEAD(rte_acl_list, rte_tailq_entry);

+static struct rte_tailq_elem rte_acl_tailq = {
+   .name = "RTE_ACL",
+};
+EAL_REGISTER_TAILQ(rte_acl_tailq)
+
 /*
  * If the compiler doesn't support AVX2 instructions,
  * then the dummy one would be used instead for AVX2 classify method.
@@ -129,12 +134,7 @@ rte_acl_find_existing(const char *name)
struct rte_acl_list *acl_list;
struct rte_tailq_entry *te;

-   /* check that we have an initialised tail queue */
-   acl_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_ACL, rte_acl_list);
-   if (acl_list == NULL) {
-   rte_errno = E_RTE_NO_TAILQ;
-   return NULL;
-   }
+   acl_list = RTE_TAILQ_CAST(rte_acl_tailq.head, rte_acl_list);

rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
TAILQ_FOREACH(te, acl_list, next) {
@@ -160,12 +160,7 @@ rte_acl_free(struct rte_acl_ctx *ctx)
if (ctx == NULL)
return;

-   /* check that we have an initialised tail queue */
-   acl_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_ACL, rte_acl_list);
-   if (acl_list == NULL) {
-   rte_errno = E_RTE_NO_TAILQ;
-   return;
-   }
+   acl_list = RTE_TAILQ_CAST(rte_acl_tailq.head, rte_acl_list);

rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);

@@ -197,12 +192,7 @@ rte_acl_create(const struct rte_acl_param *param)
struct rte_tailq_entry *te;
char name[sizeof(ctx->name)];

-   /* check that we have an initialised tail queue */
-   acl_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_ACL, rte_acl_list);
-   if (acl_list == NULL) {
-   rte_errno = E_RTE_NO_TAILQ;
-   return NULL;
-   }
+   acl_list = RTE_TAILQ_CAST(rte_acl_tailq.head, rte_acl_list);

/* check that input parameters are valid. */
if (param == NULL || param->name == NULL) {
@@ -365,12 +355,7 @@ rte_acl_list_dump(void)
struct rte_acl_list *acl_list;
struct rte_tailq_entry *te;

-   /* check that we have an initialised tail queue */
-   acl_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_ACL, rte_acl_list);
-   if (acl_list == NULL) {
-   rte_errno = E_RTE_NO_TAILQ;
-   return;
-   }
+   acl_list = RTE_TAILQ_CAST(rte_acl_tailq.head, rte_acl_list);

rte_rwlock_read_lock(RTE_EAL_TAILQ_RWLOCK);
TAILQ_FOREACH(te, acl_list, next) {
diff --git a/lib/librte_acl/rte_acl.h b/lib/librte_acl/rte_acl.h
index 30aea03..3a93730 100644
--- a/lib/librte_acl/rte_acl.h
+++ b/lib/librte_acl/rte_acl.h
@@ -170,7 +170,6 @@ struct rte_acl_param {
  *   Pointer to ACL context structure that is used in future ACL
  *   operations, or NULL on error, with error code set in rte_errno.
  *   Possible rte_errno errors include:
- *   - E_RTE_NO_TAILQ - no tailq list could be got for the ACL context list
  *   - EINVAL - invalid parameter passed to function
  */
 struct rte_acl_ctx *
diff --git a/lib/librte_distributor/rte_distributor.c 
b/lib/librte_distributor/rte_distributor.c
index 84e78bb..f3f778c 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -115,6 +115,11 @@ struct rte_distributor {

 TAILQ_HEAD(rte_distributor_list, rte_distributor);

+static struct 

[dpdk-dev] [PATCH 07/10] tailq: introduce dynamic register system

[David Marchand]

This register system makes it possible to reserve a tailq for the dpdk
libraries.
The "dynamic" tailqs are right after the "static" tailqs in shared mem.
Primary process is responsible for writing the tailq names, so that secondary
processes can find them.

This is a temp commit, "static" tailqs are removed after conversion of all
users in next commits.

Signed-off-by: David Marchand 
---
 app/test/test_tailq.c   |   82 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |1 +
 lib/librte_eal/common/eal_common_tailqs.c   |  130 +--
 lib/librte_eal/common/include/rte_tailq.h   |   38 +++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |1 +
 5 files changed, 219 insertions(+), 33 deletions(-)

diff --git a/app/test/test_tailq.c b/app/test/test_tailq.c
index 56656f0..c046a8a 100644
--- a/app/test/test_tailq.c
+++ b/app/test/test_tailq.c
@@ -34,6 +34,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 

@@ -49,40 +50,59 @@
return 1; \
 } while (0)

-#define DEFAULT_TAILQ (RTE_TAILQ_NUM)
+static struct rte_tailq_elem rte_dummy_tailq = {
+   .name = "dummy",
+};
+EAL_REGISTER_TAILQ(rte_dummy_tailq)
+
+static struct rte_tailq_elem rte_dummy_dyn_tailq = {
+   .name = "dummy_dyn",
+};
+static struct rte_tailq_elem rte_dummy_dyn2_tailq = {
+   .name = "dummy_dyn",
+};

 static struct rte_tailq_entry d_elem;
+static struct rte_tailq_entry d_dyn_elem;

 static int
-test_tailq_create(void)
+test_tailq_early(void)
 {
struct rte_tailq_entry_head *d_head;
-   unsigned i;

-   /* create a first tailq and check its non-null */
-   d_head = RTE_TAILQ_LOOKUP_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head);
+   d_head = RTE_TAILQ_CAST(rte_dummy_tailq.head, rte_tailq_entry_head);
if (d_head == NULL)
-   do_return("Error allocating dummy_q0\n");
+   do_return("Error %s has not been initialised\n",
+ rte_dummy_tailq.name);

-   /* check we can add an item to it
-*/
+   /* check we can add an item to it */
TAILQ_INSERT_TAIL(d_head, _elem, next);

-   /* try allocating dummy_q0 again, and check for failure */
-   if (RTE_TAILQ_LOOKUP_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head) == 
NULL)
-   do_return("Error, non-null result returned when attemption to "
-   "re-allocate a tailq\n");
+   return 0;
+}
+
+static int
+test_tailq_create(void)
+{
+   struct rte_tailq_entry_head *d_head;
+
+   /* create a tailq and check its non-null (since we are post-eal init) */
+   if ((rte_eal_tailq_register(_dummy_dyn_tailq) < 0) ||
+   (rte_dummy_dyn_tailq.head == NULL))
+   do_return("Error allocating %s\n", rte_dummy_dyn_tailq.name);
+
+   d_head = RTE_TAILQ_CAST(rte_dummy_dyn_tailq.head, rte_tailq_entry_head);

-   /* now fill up the tailq slots available and check we get an error */
-   for (i = RTE_TAILQ_NUM; i < RTE_MAX_TAILQ; i++){
-   if ((d_head = RTE_TAILQ_LOOKUP_BY_IDX(i,
-   rte_tailq_entry_head)) == NULL)
-   break;
-   }
+   /* check we can add an item to it */
+   TAILQ_INSERT_TAIL(d_head, _dyn_elem, next);

-   /* check that we had an error return before RTE_MAX_TAILQ */
-   if (i != RTE_MAX_TAILQ)
-   do_return("Error, we did not have a reservation as expected\n");
+   if (strcmp(rte_dummy_dyn2_tailq.name, rte_dummy_dyn_tailq.name))
+   do_return("Error, something is wrong in the tailq test\n");
+
+   /* try allocating again, and check for failure */
+   if (!rte_eal_tailq_register(_dummy_dyn2_tailq))
+   do_return("Error, registering the same tailq %s did not fail\n",
+ rte_dummy_dyn2_tailq.name);

return 0;
 }
@@ -94,8 +114,10 @@ test_tailq_lookup(void)
struct rte_tailq_entry_head *d_head;
struct rte_tailq_entry *d_ptr;

-   d_head = RTE_TAILQ_LOOKUP_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head);
-   if (d_head == NULL)
+   d_head = RTE_TAILQ_LOOKUP(rte_dummy_tailq.name, rte_tailq_entry_head);
+   /* rte_dummy_tailq has been registered by EAL_REGISTER_TAILQ */
+   if (d_head == NULL ||
+   d_head != RTE_TAILQ_CAST(rte_dummy_tailq.head, 
rte_tailq_entry_head))
do_return("Error with tailq lookup\n");

TAILQ_FOREACH(d_ptr, d_head, next)
@@ -103,8 +125,19 @@ test_tailq_lookup(void)
do_return("Error with tailq returned from lookup - "
"expected element not found\n");

+   d_head = RTE_TAILQ_LOOKUP(rte_dummy_dyn_tailq.name, 
rte_tailq_entry_head);
+   /* rte_dummy_dyn_tailq has been registered by test_tailq_create */
+   if (d_head == NULL ||
+   d_head != RTE_TAILQ_CAST(rte_dummy_dyn_tailq.head, 
rte_tailq_entry_head))
+  

[dpdk-dev] [PATCH 06/10] tailq: remove unused RTE_EAL_TAILQ_* macros

[David Marchand]

A lot of places just protect against concurrent access and I can not see the
gain of having those macros.

Signed-off-by: David Marchand 
---
 lib/librte_eal/common/include/rte_eal.h |   58 ---
 lib/librte_mempool/rte_mempool.c|   10 --
 2 files changed, 7 insertions(+), 61 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_eal.h 
b/lib/librte_eal/common/include/rte_eal.h
index b72606b..1385a73 100644
--- a/lib/librte_eal/common/include/rte_eal.h
+++ b/lib/librte_eal/common/include/rte_eal.h
@@ -195,64 +195,6 @@ rte_set_application_usage_hook( rte_usage_hook_t 
usage_func );
  */
 #define RTE_EAL_MEMPOOL_RWLOCK
(_eal_get_configuration()->mem_config->mplock)

-
-/**
- * Utility macro to do a thread-safe tailq 'INSERT' of rte_mem_config
- *
- * @param idx
- *   a kind of tailq define in enum rte_tailq_t
- *
- * @param type
- *   type of list(tailq head)
- *
- * @param elm
- *   The element will be added into the list
- *
- */
-#define RTE_EAL_TAILQ_INSERT_TAIL(idx, type, elm) do { \
-   struct type *list;  \
-   list = RTE_TAILQ_LOOKUP_BY_IDX(idx, type);  \
-   rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);\
-   TAILQ_INSERT_TAIL(list, elm, next); \
-   rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);  \
-} while (0)
-
-/**
- * Utility macro to do a thread-safe tailq 'REMOVE' of rte_mem_config
- *
- * @param idx
- *   a kind of tailq define in enum rte_tailq_t
- *
- * @param type
- *   type of list(tailq head)
- *
- * @param elm
- *   The element will be remove from the list
- *
- */
-#define RTE_EAL_TAILQ_REMOVE(idx, type, elm) do {  \
-   struct type *list;  \
-   list = RTE_TAILQ_LOOKUP_BY_IDX(idx, type);  \
-   rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);\
-   TAILQ_REMOVE(list, elm, next);  \
-   rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);  \
-} while (0) \
-
-
-/**
- *  macro to check TAILQ exist
- *
- *  @param idx
- *a kind of tailq define in enum rte_tailq_t
- *
- */
-#define RTE_EAL_TAILQ_EXIST_CHECK(idx) do {   \
-   if (RTE_TAILQ_LOOKUP_BY_IDX(idx, rte_tailq_head) == NULL){  \
-   rte_errno = E_RTE_NO_TAILQ; \
-   return NULL;\
-   }   \
-} while(0)
-
 /**
  * Whether EAL is using huge pages (disabled by --no-huge option).
  * The no-huge mode cannot be used with UIO poll-mode drivers like igb/ixgbe.
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index bb40523..3301e97 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -403,6 +403,7 @@ rte_mempool_xmem_create(const char *name, unsigned n, 
unsigned elt_size,
 {
char mz_name[RTE_MEMZONE_NAMESIZE];
char rg_name[RTE_RING_NAMESIZE];
+   struct rte_mempool_list *mempool_list;
struct rte_mempool *mp = NULL;
struct rte_tailq_entry *te;
struct rte_ring *r;
@@ -432,8 +433,9 @@ rte_mempool_xmem_create(const char *name, unsigned n, 
unsigned elt_size,
 #endif

/* check that we have an initialised tail queue */
-   if (RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_MEMPOOL,
-   rte_mempool_list) == NULL) {
+   mempool_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_MEMPOOL,
+  rte_mempool_list);
+   if (mempool_list == NULL) {
rte_errno = E_RTE_NO_TAILQ;
return NULL;
}
@@ -599,7 +601,9 @@ rte_mempool_xmem_create(const char *name, unsigned n, 
unsigned elt_size,

te->data = (void *) mp;

-   RTE_EAL_TAILQ_INSERT_TAIL(RTE_TAILQ_MEMPOOL, rte_mempool_list, te);
+   rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
+   TAILQ_INSERT_TAIL(mempool_list, te, next);
+   rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);

 exit:
rte_rwlock_write_unlock(RTE_EAL_MEMPOOL_RWLOCK);
-- 
1.7.10.4

[dpdk-dev] [PATCH 05/10] tailq: get rid of broken "reserve" api

[David Marchand]

The "reserve" macros and functions do not check if the requested entry is free.
They do nothing more than the lookup function (which itself "creates" entries
...).
The rte_tailq api is marked as "internal use" in documentation and these macros
are only used in test application, so just get rid of them.

Signed-off-by: David Marchand 
---
 app/test/test_tailq.c   |   48 +--
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |2 -
 lib/librte_eal/common/eal_common_tailqs.c   |   12 
 lib/librte_eal/common/include/rte_tailq.h   |   75 ++-
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |2 -
 5 files changed, 9 insertions(+), 130 deletions(-)

diff --git a/app/test/test_tailq.c b/app/test/test_tailq.c
index b0f9a78..56656f0 100644
--- a/app/test/test_tailq.c
+++ b/app/test/test_tailq.c
@@ -60,7 +60,7 @@ test_tailq_create(void)
unsigned i;

/* create a first tailq and check its non-null */
-   d_head = RTE_TAILQ_RESERVE_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head);
+   d_head = RTE_TAILQ_LOOKUP_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head);
if (d_head == NULL)
do_return("Error allocating dummy_q0\n");

@@ -69,13 +69,13 @@ test_tailq_create(void)
TAILQ_INSERT_TAIL(d_head, _elem, next);

/* try allocating dummy_q0 again, and check for failure */
-   if (RTE_TAILQ_RESERVE_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head) == 
NULL)
+   if (RTE_TAILQ_LOOKUP_BY_IDX(DEFAULT_TAILQ, rte_tailq_entry_head) == 
NULL)
do_return("Error, non-null result returned when attemption to "
"re-allocate a tailq\n");

/* now fill up the tailq slots available and check we get an error */
for (i = RTE_TAILQ_NUM; i < RTE_MAX_TAILQ; i++){
-   if ((d_head = RTE_TAILQ_RESERVE_BY_IDX(i,
+   if ((d_head = RTE_TAILQ_LOOKUP_BY_IDX(i,
rte_tailq_entry_head)) == NULL)
break;
}
@@ -111,54 +111,12 @@ test_tailq_lookup(void)
return 0;
 }

-/* test for deprecated functions - mainly for coverage */
-static int
-test_tailq_deprecated(void)
-{
-   struct rte_tailq_entry_head *d_head;
-
-   /* since TAILQ_RESERVE is not able to create new tailqs,
-* we should find an existing one (IOW, RTE_TAILQ_RESERVE behaves 
identical
-* to RTE_TAILQ_LOOKUP).
-*
-* PCI_RESOURCE_LIST tailq is guaranteed to
-* be present in any DPDK app. */
-   d_head = RTE_TAILQ_RESERVE("PCI_RESOURCE_LIST", rte_tailq_entry_head);
-   if (d_head == NULL)
-   do_return("Error finding PCI_RESOURCE_LIST\n");
-
-   d_head = RTE_TAILQ_LOOKUP("PCI_RESOURCE_LIST", rte_tailq_entry_head);
-   if (d_head == NULL)
-   do_return("Error finding PCI_RESOURCE_LIST\n");
-
-   /* try doing that with non-existent names */
-   d_head = RTE_TAILQ_RESERVE("random name", rte_tailq_entry_head);
-   if (d_head != NULL)
-   do_return("Non-existent tailq found!\n");
-
-   d_head = RTE_TAILQ_LOOKUP("random name", rte_tailq_entry_head);
-   if (d_head != NULL)
-   do_return("Non-existent tailq found!\n");
-
-   /* try doing the same with NULL names */
-   d_head = RTE_TAILQ_RESERVE(NULL, rte_tailq_entry_head);
-   if (d_head != NULL)
-   do_return("NULL tailq found!\n");
-
-   d_head = RTE_TAILQ_LOOKUP(NULL, rte_tailq_entry_head);
-   if (d_head != NULL)
-   do_return("NULL tailq found!\n");
-
-   return 0;
-}
-
 static int
 test_tailq(void)
 {
int ret = 0;
ret |= test_tailq_create();
ret |= test_tailq_lookup();
-   ret |= test_tailq_deprecated();
return ret;
 }

diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map 
b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
index d83524d..c94fe8e 100644
--- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
+++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
@@ -45,8 +45,6 @@ DPDK_2.0 {
rte_eal_remote_launch;
rte_eal_tailq_lookup;
rte_eal_tailq_lookup_by_idx;
-   rte_eal_tailq_reserve;
-   rte_eal_tailq_reserve_by_idx;
rte_eal_wait_lcore;
rte_exit;
rte_get_hpet_cycles;
diff --git a/lib/librte_eal/common/eal_common_tailqs.c 
b/lib/librte_eal/common/eal_common_tailqs.c
index a61e3f3..975ee74 100644
--- a/lib/librte_eal/common/eal_common_tailqs.c
+++ b/lib/librte_eal/common/eal_common_tailqs.c
@@ -94,18 +94,6 @@ rte_eal_tailq_lookup_by_idx(const unsigned tailq_idx)
return >tailq_head[tailq_idx];
 }

-struct rte_tailq_head *
-rte_eal_tailq_reserve(const char *name)
-{
-   return rte_eal_tailq_lookup(name);
-}
-
-inline struct rte_tailq_head *
-rte_eal_tailq_reserve_by_idx(const unsigned tailq_idx)
-{
-   return rte_eal_tailq_lookup_by_idx(tailq_idx);
-}
-
 void
 rte_dump_tailq(FILE *f)
 {
diff --git 

[dpdk-dev] [PATCH 03/10] tailq: remove unneeded inclusion of rte_tailq.h

[David Marchand]

Only keep inclusion where really needed.

Signed-off-by: David Marchand 
---
 app/test-pipeline/config.c |1 -
 app/test-pipeline/init.c   |1 -
 app/test-pipeline/main.c   |1 -
 app/test-pipeline/runtime.c|1 -
 app/test-pmd/cmdline.c |1 -
 app/test-pmd/config.c  |1 -
 app/test-pmd/csumonly.c|1 -
 app/test-pmd/flowgen.c |1 -
 app/test-pmd/ieee1588fwd.c |1 -
 app/test-pmd/iofwd.c   |1 -
 app/test-pmd/macfwd-retry.c|1 -
 app/test-pmd/macfwd.c  |1 -
 app/test-pmd/macswap.c |1 -
 app/test-pmd/parameters.c  |1 -
 app/test-pmd/rxonly.c  |1 -
 app/test-pmd/testpmd.c |1 -
 app/test-pmd/txonly.c  |1 -
 app/test/commands.c|1 -
 app/test/test.c|1 -
 app/test/test_atomic.c |1 -
 app/test/test_func_reentrancy.c|1 -
 app/test/test_hash.c   |1 -
 app/test/test_hash_perf.c  |1 -
 app/test/test_logs.c   |1 -
 app/test/test_malloc.c |1 -
 app/test/test_mbuf.c   |1 -
 app/test/test_mempool.c|1 -
 app/test/test_mempool_perf.c   |1 -
 app/test/test_memzone.c|1 -
 app/test/test_mp_secondary.c   |1 -
 app/test/test_per_lcore.c  |1 -
 app/test/test_ring.c   |1 -
 app/test/test_rwlock.c |1 -
 app/test/test_spinlock.c   |1 -
 app/test/test_tailq.c  |1 -
 app/test/test_timer.c  |1 -
 examples/bond/main.c   |1 -
 examples/cmdline/main.c|1 -
 examples/dpdk_qat/crypto.c |1 -
 examples/dpdk_qat/main.c   |1 -
 examples/exception_path/main.c |1 -
 examples/helloworld/main.c |1 -
 examples/ip_fragmentation/main.c   |1 -
 examples/ip_pipeline/config.c  |1 -
 examples/ip_pipeline/init.c|1 -
 examples/ip_pipeline/main.c|1 -
 examples/ip_reassembly/main.c  |1 -
 examples/ipv4_multicast/main.c |1 -
 examples/kni/main.c|1 -
 examples/l2fwd-ivshmem/guest/guest.c   |1 -
 examples/l2fwd-jobstats/main.c |1 -
 examples/l2fwd/main.c  |1 -
 examples/l3fwd-acl/main.c  |1 -
 examples/l3fwd-power/main.c|1 -
 examples/l3fwd-vf/main.c   |1 -
 examples/l3fwd/main.c  |1 -
 examples/link_status_interrupt/main.c  |1 -
 examples/load_balancer/config.c|1 -
 examples/load_balancer/init.c  |1 -
 examples/load_balancer/main.c  |1 -
 examples/load_balancer/runtime.c   |1 -
 examples/multi_process/client_server_mp/mp_client/client.c |1 -
 examples/multi_process/client_server_mp/mp_server/init.c   |1 -
 examples/multi_process/client_server_mp/mp_server/main.c   |1 -
 examples/multi_process/l2fwd_fork/flib.c   |1 -
 examples/multi_process/l2fwd_fork/main.c   |1 -
 examples/multi_process/simple_mp/main.c|1 -
 examples/multi_process/simple_mp/mp_commands.c |1 -
 examples/multi_process/symmetric_mp/main.c |1 -
 examples/timer/main.c  |1 -
 examples/vhost_xen/xen_vhost.h |1 -
 

[dpdk-dev] [PATCH 02/10] pci: use lookup tailq api

[David Marchand]

There is no reason why we should use the "reserve" tailq api, since the pci
entry is already statically reserved.

Signed-off-by: David Marchand 
---
 lib/librte_eal/bsdapp/eal/eal_pci.c   |2 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c |4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c 
b/lib/librte_eal/bsdapp/eal/eal_pci.c
index e692b35..4d2fbbb 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -493,7 +493,7 @@ rte_eal_pci_init(void)
 {
TAILQ_INIT(_driver_list);
TAILQ_INIT(_device_list);
-   uio_res_list = RTE_TAILQ_RESERVE_BY_IDX(RTE_TAILQ_PCI, uio_res_list);
+   uio_res_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_PCI, uio_res_list);

/* for debug purposes, PCI can be disabled */
if (internal_config.no_pci)
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c 
b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 6d4932d..8f44f09 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -766,8 +766,8 @@ rte_eal_pci_init(void)
 {
TAILQ_INIT(_driver_list);
TAILQ_INIT(_device_list);
-   pci_res_list = RTE_TAILQ_RESERVE_BY_IDX(RTE_TAILQ_PCI,
-   mapped_pci_res_list);
+   pci_res_list = RTE_TAILQ_LOOKUP_BY_IDX(RTE_TAILQ_PCI,
+  mapped_pci_res_list);

/* for debug purposes, PCI can be disabled */
if (internal_config.no_pci)
-- 
1.7.10.4

[dpdk-dev] [PATCH 01/10] eal: remove yet another remaining reference to pm

[David Marchand]

Hopefully, this is the last reference to pm.

Signed-off-by: David Marchand 
---
 lib/librte_eal/common/include/rte_tailq_elem.h |2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_tailq_elem.h 
b/lib/librte_eal/common/include/rte_tailq_elem.h
index 3013869..01fc769 100644
--- a/lib/librte_eal/common/include/rte_tailq_elem.h
+++ b/lib/librte_eal/common/include/rte_tailq_elem.h
@@ -78,8 +78,6 @@ rte_tailq_elem(RTE_TAILQ_LPM, "RTE_LPM")

 rte_tailq_elem(RTE_TAILQ_LPM6, "RTE_LPM6")

-rte_tailq_elem(RTE_TAILQ_PM, "RTE_PM")
-
 rte_tailq_elem(RTE_TAILQ_ACL, "RTE_ACL")

 rte_tailq_elem(RTE_TAILQ_DISTRIBUTOR, "RTE_DISTRIBUTOR")
-- 
1.7.10.4

[dpdk-dev] [PATCH 00/10] eal: rte_tailq api cleanup

[David Marchand]

This is a first cleanup at trying to remove references to other dpdk libraries
from eal.

This cleanup is focused on rte_tailq api which has been marked as "for internal
use" for quite some time now.
Rather than have a static list in eal for all users of rte_tailq, a new register
system is introduced.
This register system uses constructors which have no access to dpdk shared
memory, so a two step registration is done: first step inserts the requested
tailq in a local list ("local" in multi process context), then in second step,
eal init allocates/looks up for a real tailq from shared memory for all elements
of this local list.

I have tried to think of different cases (libraries loaded before/after eal
init...). The unit tests have been updated accordingly.


-- 
David Marchand

David Marchand (10):
  eal: remove yet another remaining reference to pm
  pci: use lookup tailq api
  tailq: remove unneeded inclusion of rte_tailq.h
  tailq: use a single cast macro
  tailq: get rid of broken "reserve" api
  tailq: remove unused RTE_EAL_TAILQ_* macros
  tailq: introduce dynamic register system
  tailq: move to dynamic tailq
  tailq: remove static slots
  eal: no need for E_RTE_NO_TAILQ anymore

 app/test-pipeline/config.c |1 -
 app/test-pipeline/init.c   |1 -
 app/test-pipeline/main.c   |1 -
 app/test-pipeline/runtime.c|1 -
 app/test-pmd/cmdline.c |1 -
 app/test-pmd/config.c  |1 -
 app/test-pmd/csumonly.c|1 -
 app/test-pmd/flowgen.c |1 -
 app/test-pmd/ieee1588fwd.c |1 -
 app/test-pmd/iofwd.c   |1 -
 app/test-pmd/macfwd-retry.c|1 -
 app/test-pmd/macfwd.c  |1 -
 app/test-pmd/macswap.c |1 -
 app/test-pmd/parameters.c  |1 -
 app/test-pmd/rxonly.c  |1 -
 app/test-pmd/testpmd.c |1 -
 app/test-pmd/txonly.c  |1 -
 app/test/commands.c|1 -
 app/test/test.c|1 -
 app/test/test_atomic.c |1 -
 app/test/test_errno.c  |2 +-
 app/test/test_func_reentrancy.c|1 -
 app/test/test_hash.c   |1 -
 app/test/test_hash_perf.c  |1 -
 app/test/test_logs.c   |1 -
 app/test/test_malloc.c |1 -
 app/test/test_mbuf.c   |1 -
 app/test/test_mempool.c|1 -
 app/test/test_mempool_perf.c   |1 -
 app/test/test_memzone.c|1 -
 app/test/test_mp_secondary.c   |1 -
 app/test/test_per_lcore.c  |1 -
 app/test/test_ring.c   |1 -
 app/test/test_rwlock.c |1 -
 app/test/test_spinlock.c   |1 -
 app/test/test_tailq.c  |  125 
 app/test/test_timer.c  |1 -
 examples/bond/main.c   |1 -
 examples/cmdline/main.c|1 -
 examples/dpdk_qat/crypto.c |1 -
 examples/dpdk_qat/main.c   |1 -
 examples/exception_path/main.c |1 -
 examples/helloworld/main.c |1 -
 examples/ip_fragmentation/main.c   |1 -
 examples/ip_pipeline/config.c  |1 -
 examples/ip_pipeline/init.c|1 -
 examples/ip_pipeline/main.c|1 -
 examples/ip_reassembly/main.c  |1 -
 examples/ipv4_multicast/main.c |1 -
 examples/kni/main.c|1 -
 examples/l2fwd-ivshmem/guest/guest.c   |1 -
 examples/l2fwd-jobstats/main.c |1 -
 examples/l2fwd/main.c  |1 -
 examples/l3fwd-acl/main.c  |1 -
 examples/l3fwd-power/main.c|1 -
 examples/l3fwd-vf/main.c   |1 -
 examples/l3fwd/main.c  |1 -
 examples/link_status_interrupt/main.c  |1 -
 examples/load_balancer/config.c|1 -
 examples/load_balancer/init.c  |1 -
 examples/load_balancer/main.c  |1 -
 examples/load_balancer/runtime.c   |  

[dpdk-dev] [PATCH] ixgbe: do not include CRC in Tx byte count

[Thomas Monjalon]

Anyone to carefully review this patch?

2015-01-27 11:38, Stephen Hemminger:
> On Tue, 27 Jan 2015 11:11:39 +0100
> Thomas Monjalon  wrote:
> 
> > Hi Stephen,
> > 
> > 2015-01-22 22:23, stephen at networkplumber.org:
> > > From: Stephen Hemminger 
> > > 
> > > The ixgbe driver was including CRC in the transmit packet byte
> > > count, but not for packets received. This was notice when forwarding and
> > > the number of bytes received was greater than the number of bytes 
> > > transmitted
> > 
> > Tx includes CRC and Rx count is greater, really?

ping

> > > for the same number of packets. Make the driver behave like other
> > > virtual devices and not include CRC in byte count. Use the same queue
> > > counters already computed and used for Rx.
> > 
> > Please could you describe the difference between gptc/gotc and qptc/qbtc?
> > 
> > Thank you
> 
> 
> The byte counts for global registers include CRC in the byte count.
> This has been observed by experimentation and validated by QA and documented
> in Intel HW specs.
> 
> The original code used queue counts for Rx because the global counters
> include missed packets (which must not be included in the ipacket counts).
> I suspect that is why the original developer used the queue counts, as
> a good side effect the Rx byte count was correct (no CRC). I just extended
> this for Tx.

[dpdk-dev] [PATCH 1/6] test: remove unneeded casts

[Thomas Monjalon]

2015-02-19 14:53, Bruce Richardson:
> On Sat, Feb 14, 2015 at 09:59:05AM -0500, Stephen Hemminger wrote:
> > The malloc family returns void * and therefore cast is unnecessary.
> > Use calloc rather than zmalloc with multiply for array.
> > 
> > Signed-off-by: Stephen Hemminger 
> 
> Looks like a good basic cleanup
> 
> Acked-by: Bruce Richardson 

Applied the series despite the lack of answer to simple questions.

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Olivier MATZ]

Hi Keith,

On 03/04/2015 05:47 PM, Wiles, Keith wrote:
> Hi Olivier
>
> On 3/4/15, 10:40 AM, "Olivier MATZ"  wrote:
>
>> Hi Keith,
>>
>> On 03/04/2015 05:11 PM, Wiles, Keith wrote:
>>>
>>>
>>> On 3/4/15, 3:08 AM, "Olivier MATZ"  wrote:
>>>
 Hi Keith,

 On 02/28/2015 05:56 PM, Keith Wiles wrote:
> When building an external application like Pktgen and using the proper
> makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
> use with example applications in the same RTE_SDK directory the
> rte.extXYZ.mk
> files are missing some defines/includes.
>
>  1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
>  2 - The build of external applications are forced to be verbose
> ouput
>  as the Q=@ define is not present.
>  3 - Missing include of target/generic/rte.vars.mk file which
> includes
>  the information to locate the rte_config.h and other DPDK
> include
>  files.
>
> A patch like this one was submitted before and was rejected because it
> seemed it was not required, because target/generic/rte.vars.mk already
> included by rte.vars.mk.
>
> This is not the cause for external applications like Pktgen which are
> built outside of the RTE_SDK directory and only include the
> rte.extXYZ.mk
> makefile fragments.

 I still not understand what is your problem. If you take an example
 from
 dpdk, let's say examples/l2fwd.

 cd test
 # compile dpdk
 git clone http://dpdk.org/git/dpdk
 cd dpdk
 DPDK=${PWD}
 make -j8 install T=x86_64-native-linuxapp-gcc
 cd ..
 # copy l2fwd in an external directory
 cp -r dpdk/examples/l2fwd .
 cd l2fwd
 # build it
 make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}
>>>
>>> Yes, this very trivial example works, but only because the makefiles are
>>> combining the two different make fragments IMO.
>>>
>>> Then why do we have rte.extvars.mk fragment at all if it was not to be
>>> used for building outside the DPDK build directory?
>>> Why were the rte.extXYZ.mk make fragments created at all, but to
>>> provide a
>>> clean building system outside of DPDK build?
>>>
>>> It seem like to me we are combining two different build systems when
>>> building the examples. If rte.extvars.mk is not used then lets delete it
>>> or replace it with a single line to include rte.vars.mk.
>>>
>>> IMO combining the two different make fragment styles is confusing and we
>>> need to remove rte.extvars.mk or replace it with my changes or replace
>>> it
>>> with a single line just to include rte.vars.mk, pick one.
>>
>> The examples and the documentation say to use "rte.vars.mk" for external
>> applications. It's like this since the beginning, so changing the
>> behavior now should be done with care to avoid breaking the working
>> applications. I don't think it's a good idea.
>>
>> I would prefer to move add rte.extvars.mk in dpdk/mk/internal to avoid
>> people doing this mistake again, what do you think?
>
> Instead of moving the file and someone using it anyway (as it is broke
> IMO) lets just replace the content of the file with a single line 'include
> rte.vars.mk' and we solve the problem. Plus this solves my symmetry
> problem :-)

I don't understand why would someone include this file directly?
What is the reason you did that at the first place?
Usually, people start from an example or the documentation, which are
both correct.

We cannot replace the content of rte.extvars.mk by an include to
rte.vars.mk because currently rte.vars.mk includes rte.extvars.mk.
To me, the only problem is that rte.extvars.mk should be marked as
internal.

Allowing to include rte.extvars.mk in dpdk to fix the behavior of
pktgen makefiles does not seem to be a good argument. Why not fixing
pktgen instead? What is broken in dpdk?

Regards,
Olivier

[dpdk-dev] [PATCH v3] ABI: Add abi checking utility

[Thomas Monjalon]

2015-03-04 11:26, Neil Horman:
> +#trap on ctrl-c to clean up
> +trap cleanup_and_exit SIGINT

I think INT is preffered over SIGINT.
You may also add QUIT and TERM.
With QUIT, you can replace cleanup_and_exit calls by a simple exit.

> + CURRENT_BRANCH=`git log --pretty=format:%H HEAD~1..HEAD`

May be simpler "git log -1 --format=%H"

> +log "INFO" "We're going to check and make sure that applications built"
> +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> +log "INFO" "against DPDK DSOs built from tag $TAG2."

I think it may be removed as no app is run.

> +# Make sure we configure SHARED libraries
> +# Also turn off IGB and KNI as those require kernel headers to build
> +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET

So you prefer modifying defconfig instead of .config, right?
(you sent it while I was answering on v2)

> +# Checking abi compliance relies on using the dwarf information in
> +# The shared objects.  Thats only included in the DSO's if we build
> +# with -g
> +export EXTRA_CFLAGS=-g
> +export EXTRA_LDFLAGS=-g
[...]
> +export EXTRA_CFLAGS=-g
> +export EXTRA_LDFLAGS=-g

Already exported.

> + OLDNAME=`basename $i | sed -e"s/1.dump/0.dump/"`

Could be OLDNAME=$(basename $i 1.dump)0.dump

> + LIBNAME=`basename $i | sed -e"s/-ABI-1.dump//"`

Could be LIBNAME=$(basename $i -ABI-1.dump)

Thanks

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Olivier MATZ]

Hi Keith,

On 03/04/2015 05:11 PM, Wiles, Keith wrote:
>
>
> On 3/4/15, 3:08 AM, "Olivier MATZ"  wrote:
>
>> Hi Keith,
>>
>> On 02/28/2015 05:56 PM, Keith Wiles wrote:
>>> When building an external application like Pktgen and using the proper
>>> makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
>>> use with example applications in the same RTE_SDK directory the
>>> rte.extXYZ.mk
>>> files are missing some defines/includes.
>>>
>>> 1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
>>> 2 - The build of external applications are forced to be verbose ouput
>>> as the Q=@ define is not present.
>>> 3 - Missing include of target/generic/rte.vars.mk file which includes
>>> the information to locate the rte_config.h and other DPDK include
>>> files.
>>>
>>> A patch like this one was submitted before and was rejected because it
>>> seemed it was not required, because target/generic/rte.vars.mk already
>>> included by rte.vars.mk.
>>>
>>> This is not the cause for external applications like Pktgen which are
>>> built outside of the RTE_SDK directory and only include the
>>> rte.extXYZ.mk
>>> makefile fragments.
>>
>> I still not understand what is your problem. If you take an example from
>> dpdk, let's say examples/l2fwd.
>>
>>cd test
>># compile dpdk
>>git clone http://dpdk.org/git/dpdk
>>cd dpdk
>>DPDK=${PWD}
>>make -j8 install T=x86_64-native-linuxapp-gcc
>>cd ..
>># copy l2fwd in an external directory
>>cp -r dpdk/examples/l2fwd .
>>cd l2fwd
>># build it
>>make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}
>
> Yes, this very trivial example works, but only because the makefiles are
> combining the two different make fragments IMO.
>
> Then why do we have rte.extvars.mk fragment at all if it was not to be
> used for building outside the DPDK build directory?
> Why were the rte.extXYZ.mk make fragments created at all, but to provide a
> clean building system outside of DPDK build?
>
> It seem like to me we are combining two different build systems when
> building the examples. If rte.extvars.mk is not used then lets delete it
> or replace it with a single line to include rte.vars.mk.
>
> IMO combining the two different make fragment styles is confusing and we
> need to remove rte.extvars.mk or replace it with my changes or replace it
> with a single line just to include rte.vars.mk, pick one.

The examples and the documentation say to use "rte.vars.mk" for external
applications. It's like this since the beginning, so changing the
behavior now should be done with care to avoid breaking the working
applications. I don't think it's a good idea.

I would prefer to move add rte.extvars.mk in dpdk/mk/internal to avoid
people doing this mistake again, what do you think?

Regards,
Olivier

[dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Thomas Monjalon]

2015-03-04 10:42, Neil Horman:
> On Wed, Mar 04, 2015 at 04:15:18PM +0100, Thomas Monjalon wrote:
> > 2015-03-04 09:39, Neil Horman:
> > > On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> > > > Hi Neil,
> > > > 
> > > > I remove parts that I agree and reply to those which deserve more 
> > > > discussion.
> > > > 
> > > > 2015-03-04 06:49, Neil Horman:
> > > > > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > > > > 2015-02-02 13:18, Neil Horman:
> > > > > > > +# Make sure we configure SHARED libraries
> > > > > > > +# Also turn off IGB and KNI as those require kernel headers to 
> > > > > > > build
> > > > > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" 
> > > > > > > config/defconfig_$TARGET
> > > > > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > > > > 
> > > > > > Why not tuning configuration after make config in .config file?
> > > > > > 
> > > > > Because this way we save a reconfig (from a developer viewpoint), you 
> > > > > should run
> > > > > make config again after changing configs, and so this way you save 
> > > > > doing that.
> > > > 
> > > > No, you run make config once and update .config file. That's the 
> > > > recommended
> > > > way to configure DPDK.
> > > > defconfig files are default configurations and should stay read-only.
> > > 
> > > They get overwritten when we do the git resets.  Its silly to modify your 
> > > config
> > > file after you run make config, in the event the make target has to 
> > > re-read any
> > > modified options and adjust dependent config files accordingly.  I 
> > > understand
> > > that doesn't happen now, but its common practice for every open source 
> > > project
> > > in existance.
> > 
> > I'm not sure to understand. Maybe an example would help.
> > By the way, your method works.
> 
> For example, the linux kernel.  The .config file that is generated in the root
> directory is converted to an autoconf.h in parallel with its generation, for
> applications to key off of.  If you change something in .config, you need to 
> run
> make config again so that those changes are reflected into the other
> auto-generated files.  Thats common practice.  So its counter intuitive to
> assume that altering the generated .config file is automatically recognized by
> the rest of the build, without a subsequent make config (be it explicit or and
> implicit dependency of the make all target).

OK thanks, now I better understand how you think about DPDK config.
Note that in Linux you are modifying .config, not the defconfig.
I'm not going to debate how it could be improved now but I think we shouldn't
dynamically modify defconfig files to avoid confusion about their purpose.

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Wiles, Keith]

Hi Olivier,

On 3/4/15, 11:04 AM, "Olivier MATZ"  wrote:

>Hi Keith,
>
>On 03/04/2015 05:47 PM, Wiles, Keith wrote:
>> Hi Olivier
>>
>> On 3/4/15, 10:40 AM, "Olivier MATZ"  wrote:
>>
>>> Hi Keith,
>>>
>>> On 03/04/2015 05:11 PM, Wiles, Keith wrote:


 On 3/4/15, 3:08 AM, "Olivier MATZ"  wrote:

> Hi Keith,
>
> On 02/28/2015 05:56 PM, Keith Wiles wrote:
>> When building an external application like Pktgen and using the
>>proper
>> makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
>> use with example applications in the same RTE_SDK directory the
>> rte.extXYZ.mk
>> files are missing some defines/includes.
>>
>>  1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
>>  2 - The build of external applications are forced to be verbose
>> ouput
>>  as the Q=@ define is not present.
>>  3 - Missing include of target/generic/rte.vars.mk file which
>> includes
>>  the information to locate the rte_config.h and other DPDK
>> include
>>  files.
>>
>> A patch like this one was submitted before and was rejected because
>>it
>> seemed it was not required, because target/generic/rte.vars.mk
>>already
>> included by rte.vars.mk.
>>
>> This is not the cause for external applications like Pktgen which
>>are
>> built outside of the RTE_SDK directory and only include the
>> rte.extXYZ.mk
>> makefile fragments.
>
> I still not understand what is your problem. If you take an example
> from
> dpdk, let's say examples/l2fwd.
>
> cd test
> # compile dpdk
> git clone http://dpdk.org/git/dpdk
> cd dpdk
> DPDK=${PWD}
> make -j8 install T=x86_64-native-linuxapp-gcc
> cd ..
> # copy l2fwd in an external directory
> cp -r dpdk/examples/l2fwd .
> cd l2fwd
> # build it
> make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}

 Yes, this very trivial example works, but only because the makefiles
are
 combining the two different make fragments IMO.

 Then why do we have rte.extvars.mk fragment at all if it was not to be
 used for building outside the DPDK build directory?
 Why were the rte.extXYZ.mk make fragments created at all, but to
 provide a
 clean building system outside of DPDK build?

 It seem like to me we are combining two different build systems when
 building the examples. If rte.extvars.mk is not used then lets delete
it
 or replace it with a single line to include rte.vars.mk.

 IMO combining the two different make fragment styles is confusing and
we
 need to remove rte.extvars.mk or replace it with my changes or replace
 it
 with a single line just to include rte.vars.mk, pick one.
>>>
>>> The examples and the documentation say to use "rte.vars.mk" for
>>>external
>>> applications. It's like this since the beginning, so changing the
>>> behavior now should be done with care to avoid breaking the working
>>> applications. I don't think it's a good idea.
>>>
>>> I would prefer to move add rte.extvars.mk in dpdk/mk/internal to avoid
>>> people doing this mistake again, what do you think?
>>
>> Instead of moving the file and someone using it anyway (as it is broke
>> IMO) lets just replace the content of the file with a single line
>>'include
>> rte.vars.mk' and we solve the problem. Plus this solves my symmetry
>> problem :-)
>
>I don't understand why would someone include this file directly?
>What is the reason you did that at the first place?
>Usually, people start from an example or the documentation, which are
>both correct.
>
>We cannot replace the content of rte.extvars.mk by an include to
>rte.vars.mk because currently rte.vars.mk includes rte.extvars.mk.
>To me, the only problem is that rte.extvars.mk should be marked as
>internal.
>
>Allowing to include rte.extvars.mk in dpdk to fix the behavior of
>pktgen makefiles does not seem to be a good argument. Why not fixing
>pktgen instead? What is broken in dpdk?

I just posted your point before you finished you email and will submit a
patch to move rte.extvars.mk to mk/rte.extvars.mk would that work?
>
>Regards,
>Olivier
>

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Wiles, Keith]

On 3/4/15, 10:47 AM, "Wiles, Keith"  wrote:

>Hi Olivier
>
>On 3/4/15, 10:40 AM, "Olivier MATZ"  wrote:
>
>>Hi Keith,
>>
>>On 03/04/2015 05:11 PM, Wiles, Keith wrote:
>>>
>>>
>>> On 3/4/15, 3:08 AM, "Olivier MATZ"  wrote:
>>>
 Hi Keith,

 On 02/28/2015 05:56 PM, Keith Wiles wrote:
> When building an external application like Pktgen and using the
>proper
> makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
> use with example applications in the same RTE_SDK directory the
> rte.extXYZ.mk
> files are missing some defines/includes.
>
> 1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
> 2 - The build of external applications are forced to be verbose
>ouput
> as the Q=@ define is not present.
> 3 - Missing include of target/generic/rte.vars.mk file which
>includes
> the information to locate the rte_config.h and other DPDK
>include
> files.
>
> A patch like this one was submitted before and was rejected because
>it
> seemed it was not required, because target/generic/rte.vars.mk
>already
> included by rte.vars.mk.
>
> This is not the cause for external applications like Pktgen which are
> built outside of the RTE_SDK directory and only include the
> rte.extXYZ.mk
> makefile fragments.

 I still not understand what is your problem. If you take an example
from
 dpdk, let's say examples/l2fwd.

cd test
# compile dpdk
git clone http://dpdk.org/git/dpdk
cd dpdk
DPDK=${PWD}
make -j8 install T=x86_64-native-linuxapp-gcc
cd ..
# copy l2fwd in an external directory
cp -r dpdk/examples/l2fwd .
cd l2fwd
# build it
make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}
>>>
>>> Yes, this very trivial example works, but only because the makefiles
>>>are
>>> combining the two different make fragments IMO.
>>>
>>> Then why do we have rte.extvars.mk fragment at all if it was not to be
>>> used for building outside the DPDK build directory?
>>> Why were the rte.extXYZ.mk make fragments created at all, but to
>>>provide a
>>> clean building system outside of DPDK build?
>>>
>>> It seem like to me we are combining two different build systems when
>>> building the examples. If rte.extvars.mk is not used then lets delete
>>>it
>>> or replace it with a single line to include rte.vars.mk.
>>>
>>> IMO combining the two different make fragment styles is confusing and
>>>we
>>> need to remove rte.extvars.mk or replace it with my changes or replace
>>>it
>>> with a single line just to include rte.vars.mk, pick one.
>>
>>The examples and the documentation say to use "rte.vars.mk" for external
>>applications. It's like this since the beginning, so changing the
>>behavior now should be done with care to avoid breaking the working
>>applications. I don't think it's a good idea.
>>
>>I would prefer to move add rte.extvars.mk in dpdk/mk/internal to avoid
>>people doing this mistake again, what do you think?
>
>Instead of moving the file and someone using it anyway (as it is broke
>IMO) lets just replace the content of the file with a single line 'include
>rte.vars.mk' and we solve the problem. Plus this solves my symmetry
>problem :-)

The file can not be replaced with a single include line as it already
includes rte.extvars.mk :-(

OK lets just move it to mk/internal and I will submit a patch for that
change.
>
>Regards
>++Keith
>>
>>Regards,
>>Olivier
>>
>

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Wiles, Keith]

Hi Olivier

On 3/4/15, 10:40 AM, "Olivier MATZ"  wrote:

>Hi Keith,
>
>On 03/04/2015 05:11 PM, Wiles, Keith wrote:
>>
>>
>> On 3/4/15, 3:08 AM, "Olivier MATZ"  wrote:
>>
>>> Hi Keith,
>>>
>>> On 02/28/2015 05:56 PM, Keith Wiles wrote:
 When building an external application like Pktgen and using the proper
 makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
 use with example applications in the same RTE_SDK directory the
 rte.extXYZ.mk
 files are missing some defines/includes.

 1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
 2 - The build of external applications are forced to be verbose
ouput
 as the Q=@ define is not present.
 3 - Missing include of target/generic/rte.vars.mk file which
includes
 the information to locate the rte_config.h and other DPDK
include
 files.

 A patch like this one was submitted before and was rejected because it
 seemed it was not required, because target/generic/rte.vars.mk already
 included by rte.vars.mk.

 This is not the cause for external applications like Pktgen which are
 built outside of the RTE_SDK directory and only include the
 rte.extXYZ.mk
 makefile fragments.
>>>
>>> I still not understand what is your problem. If you take an example
>>>from
>>> dpdk, let's say examples/l2fwd.
>>>
>>>cd test
>>># compile dpdk
>>>git clone http://dpdk.org/git/dpdk
>>>cd dpdk
>>>DPDK=${PWD}
>>>make -j8 install T=x86_64-native-linuxapp-gcc
>>>cd ..
>>># copy l2fwd in an external directory
>>>cp -r dpdk/examples/l2fwd .
>>>cd l2fwd
>>># build it
>>>make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}
>>
>> Yes, this very trivial example works, but only because the makefiles are
>> combining the two different make fragments IMO.
>>
>> Then why do we have rte.extvars.mk fragment at all if it was not to be
>> used for building outside the DPDK build directory?
>> Why were the rte.extXYZ.mk make fragments created at all, but to
>>provide a
>> clean building system outside of DPDK build?
>>
>> It seem like to me we are combining two different build systems when
>> building the examples. If rte.extvars.mk is not used then lets delete it
>> or replace it with a single line to include rte.vars.mk.
>>
>> IMO combining the two different make fragment styles is confusing and we
>> need to remove rte.extvars.mk or replace it with my changes or replace
>>it
>> with a single line just to include rte.vars.mk, pick one.
>
>The examples and the documentation say to use "rte.vars.mk" for external
>applications. It's like this since the beginning, so changing the
>behavior now should be done with care to avoid breaking the working
>applications. I don't think it's a good idea.
>
>I would prefer to move add rte.extvars.mk in dpdk/mk/internal to avoid
>people doing this mistake again, what do you think?

Instead of moving the file and someone using it anyway (as it is broke
IMO) lets just replace the content of the file with a single line 'include
rte.vars.mk' and we solve the problem. Plus this solves my symmetry
problem :-)

Regards
++Keith
>
>Regards,
>Olivier
>

[dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Thomas Monjalon]

2015-03-04 09:39, Neil Horman:
> On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> > Hi Neil,
> > 
> > I remove parts that I agree and reply to those which deserve more 
> > discussion.
> > 
> > 2015-03-04 06:49, Neil Horman:
> > > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > > 2015-02-02 13:18, Neil Horman:
> > > > > +# Validate that we have all the arguments we need
> > > > > +if [ ! -d ./.git ]
> > > > > +then
> > > > > + log "WARN" "You must be in the root of the dpdk git tree"
> > > > > + log "WARN" "You are in $PWD"
> > > > > + cleanup_and_exit 1
> > > > > +fi
> > > > 
> > > > Why not cd $(dirname $0)/.. instead of returning an error?
> > > 
> > > Why would that help in finding the base of the git tree.  Theres no 
> > > guarantee
> > > that you are in a subdirectory of a git tree.  I suppose we can try it
> > > recursively until we hit /, but it seems just as easy and clear to tell 
> > > the user
> > > whats needed.
> > 
> > No I'm saying that you could avoid this check by going into the right
> > directory from the beginning.
> > We know that the root dir is $(dirname $0)/.. because this script is in
> > scripts/ directory.
> > 
> That only helps if you start from the right directory.  If you run this 
> command
> from some other location, your solution just breaks.

Why it would break? $(dirname $0) is always reachable because you launched $0.
The only exception is for the case the PATH variable is used to find the DPDK
scripts/ directory (should not happen).

> > > > > +# Make sure we configure SHARED libraries
> > > > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > > 
> > > > Why not tuning configuration after make config in .config file?
> > > > 
> > > Because this way we save a reconfig (from a developer viewpoint), you 
> > > should run
> > > make config again after changing configs, and so this way you save doing 
> > > that.
> > 
> > No, you run make config once and update .config file. That's the recommended
> > way to configure DPDK.
> > defconfig files are default configurations and should stay read-only.
> 
> They get overwritten when we do the git resets.  Its silly to modify your 
> config
> file after you run make config, in the event the make target has to re-read 
> any
> modified options and adjust dependent config files accordingly.  I understand
> that doesn't happen now, but its common practice for every open source project
> in existance.

I'm not sure to understand. Maybe an example would help.
By the way, your method works.

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Wiles, Keith]

On 3/4/15, 3:08 AM, "Olivier MATZ"  wrote:

>Hi Keith,
>
>On 02/28/2015 05:56 PM, Keith Wiles wrote:
>> When building an external application like Pktgen and using the proper
>> makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
>> use with example applications in the same RTE_SDK directory the
>>rte.extXYZ.mk
>> files are missing some defines/includes.
>>
>>1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
>>2 - The build of external applications are forced to be verbose ouput
>>as the Q=@ define is not present.
>>3 - Missing include of target/generic/rte.vars.mk file which includes
>>the information to locate the rte_config.h and other DPDK include
>>files.
>>
>> A patch like this one was submitted before and was rejected because it
>> seemed it was not required, because target/generic/rte.vars.mk already
>> included by rte.vars.mk.
>>
>> This is not the cause for external applications like Pktgen which are
>> built outside of the RTE_SDK directory and only include the
>>rte.extXYZ.mk
>> makefile fragments.
>
>I still not understand what is your problem. If you take an example from
>dpdk, let's say examples/l2fwd.
>
>   cd test
>   # compile dpdk
>   git clone http://dpdk.org/git/dpdk
>   cd dpdk
>   DPDK=${PWD}
>   make -j8 install T=x86_64-native-linuxapp-gcc
>   cd ..
>   # copy l2fwd in an external directory
>   cp -r dpdk/examples/l2fwd .
>   cd l2fwd
>   # build it
>   make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}

Yes, this very trivial example works, but only because the makefiles are
combining the two different make fragments IMO.

Then why do we have rte.extvars.mk fragment at all if it was not to be
used for building outside the DPDK build directory?
Why were the rte.extXYZ.mk make fragments created at all, but to provide a
clean building system outside of DPDK build?

It seem like to me we are combining two different build systems when
building the examples. If rte.extvars.mk is not used then lets delete it
or replace it with a single line to include rte.vars.mk.

IMO combining the two different make fragment styles is confusing and we
need to remove rte.extvars.mk or replace it with my changes or replace it
with a single line just to include rte.vars.mk, pick one.

>
>So if you use a Makefile similar to l2fwd, I think it should work.
>As I explained in [1], you need to include "rte.vars.mk" at the
>beginning of the Makefile, not "rte.extvars.mk". But you will use
>"rte.extapp.mk" at the end, like in l2fwd Makefile.
>
>Regards,
>Olivier
>
>
>[1] http://dpdk.org/ml/archives/dev/2015-February/013301.html

[dpdk-dev] [PATCH] config: default to shared library

[Panu Matilainen]

On 03/04/2015 03:31 PM, Bruce Richardson wrote:
> On Wed, Mar 04, 2015 at 03:24:12PM +0200, Panu Matilainen wrote:
>> On 03/04/2015 03:08 PM, Bruce Richardson wrote:
>>> On Wed, Mar 04, 2015 at 06:28:05AM -0500, Neil Horman wrote:
 On Wed, Mar 04, 2015 at 01:05:07PM +0200, Panu Matilainen wrote:
> On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
>> Hi Panu,
>>
>> 2015-03-04 08:17, Panu Matilainen:
>>> With symbol versioning its vital that developers test their code in
>>> shared library mode, otherwise we'll be playing "add the forgotten
>>> symbol export" from here to eternity.
>>
>> Yes we must improve the sanity checks.
>> A lot of options must be tested (or removed) and not only shared libs.
>> But the error you reported before (missing export of 
>> rte_eth_dev_release_port)
>> cannot be seen even with this patch.
>
> I know, I didn't say it would have directly caught it. It would've likely
> been found earlier though, if nothing else then in testing of the new
> librte_pmd_null which clearly nobody had tried in shared lib 
> configuration.
>
 This is accurate.  The default config is a tool, in the sense that it 
 leverages
 the implicit testing of any users who are experimenting with the DPDK.  Any
 users out there using the DPDK test/example applications would have 
 realized
 something was amiss when the testpmd app refused to run with the null or 
 pcap
 pmd, since there was a missing symbol.  That "social fuzzing" has value, 
 but it
 only works if the defaults are carefully selected.  Currently, building for
 shared libraries exposes more existing bugs than static libraries, and so 
 we
 should set that as our default so as to catch them.

>> It means we need more tools.
>> Though, default configuration is not a tool.
>
> Yes, default config is not a tool, its a recommendation of sorts both for
> developers and users. It also tends to be the setup that is rarely broken
> because it happens to get the most testing :)
>
 And it is a tool (see above).

>>
>>> By defaulting to shared we should catch more of these cases early,
>>> but without taking away anybodys ability to build static.
>>
>> Shared libraries are convenient for distributions but have a performance
>> impact. I think that static build must remain the default choice.
>

 If utmost performance is the concern, isn't it reasonable to assume that 
 users
 in that demographic will customize their configuration to achieve that?  
 No one
 assumes that something is tuned to be perfect for their needs out of the 
 box if
 their needs are extreemely biased to a single quality.  The best course of
 action here is to set the default to be adventageous toward catching bugs, 
 and
 document the changes needed to bias for performance.

> For distros, this is not a matter of *convenience*, its the only 
> technically
> feasible choice.
>>>
>>> As I understand it, build for the "default" cpu rather than "native" is the 
>>> only
>>> feasible choice also, so how about re-introducing a new defconfig file for
>>> "default" (or perhaps better name), where you have lowest-common denominator
>>> instruction-set and building for shared libraries?
>>> Would that work for everyone, or do people feel it would be too confusing 
>>> to have
>>> more defconfig files available?
>>
>> Given the opposition to defaulting to shared, another config file seems like
>> a fair compromise to me, whether "default" or something else. As for the
>> naming, one possibility would be calling it "shared", implying both
>> lowest-common denominator instruction set to be shareable across many
>> systems and shared libraries.
>>
>>  - Panu -
>
> The naming scheme for configs is meant to be:
> "ARCH-MACHINE-EXECENV-TOOLCHAIN"
> as documented in the Getting Started Guide. "Default" has been used up till 
> now
> to refer to the lowest common denominator instruction set supported, which for
> x86_64 is a core2 baseline, I believe. "shared" doesn't really fit into this
> naming scheme, and there is nothing to allow extra notes to be added to the
> name.

Right, but then there's "ivshmem" that doesn't fit that description 
either AFAICS.

> Without changing this scheme, I would suggest we rename "default" to 
> "generic",
> which I think is a slightly better term for it, and we set the
> "x86_64-generic-linuxapp-gcc" target to build shared libs.

Works for me. It is indeed more descriptive than either "default" or 
"shared" for the purpose.

- Panu -

[dpdk-dev] [PATCH] config: default to shared library

[Panu Matilainen]

On 03/04/2015 03:08 PM, Bruce Richardson wrote:
> On Wed, Mar 04, 2015 at 06:28:05AM -0500, Neil Horman wrote:
>> On Wed, Mar 04, 2015 at 01:05:07PM +0200, Panu Matilainen wrote:
>>> On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
 Hi Panu,

 2015-03-04 08:17, Panu Matilainen:
> With symbol versioning its vital that developers test their code in
> shared library mode, otherwise we'll be playing "add the forgotten
> symbol export" from here to eternity.

 Yes we must improve the sanity checks.
 A lot of options must be tested (or removed) and not only shared libs.
 But the error you reported before (missing export of 
 rte_eth_dev_release_port)
 cannot be seen even with this patch.
>>>
>>> I know, I didn't say it would have directly caught it. It would've likely
>>> been found earlier though, if nothing else then in testing of the new
>>> librte_pmd_null which clearly nobody had tried in shared lib configuration.
>>>
>> This is accurate.  The default config is a tool, in the sense that it 
>> leverages
>> the implicit testing of any users who are experimenting with the DPDK.  Any
>> users out there using the DPDK test/example applications would have realized
>> something was amiss when the testpmd app refused to run with the null or pcap
>> pmd, since there was a missing symbol.  That "social fuzzing" has value, but 
>> it
>> only works if the defaults are carefully selected.  Currently, building for
>> shared libraries exposes more existing bugs than static libraries, and so we
>> should set that as our default so as to catch them.
>>
 It means we need more tools.
 Though, default configuration is not a tool.
>>>
>>> Yes, default config is not a tool, its a recommendation of sorts both for
>>> developers and users. It also tends to be the setup that is rarely broken
>>> because it happens to get the most testing :)
>>>
>> And it is a tool (see above).
>>

> By defaulting to shared we should catch more of these cases early,
> but without taking away anybodys ability to build static.

 Shared libraries are convenient for distributions but have a performance
 impact. I think that static build must remain the default choice.
>>>
>>
>> If utmost performance is the concern, isn't it reasonable to assume that 
>> users
>> in that demographic will customize their configuration to achieve that?  No 
>> one
>> assumes that something is tuned to be perfect for their needs out of the box 
>> if
>> their needs are extreemely biased to a single quality.  The best course of
>> action here is to set the default to be adventageous toward catching bugs, 
>> and
>> document the changes needed to bias for performance.
>>
>>> For distros, this is not a matter of *convenience*, its the only technically
>>> feasible choice.
>
> As I understand it, build for the "default" cpu rather than "native" is the 
> only
> feasible choice also, so how about re-introducing a new defconfig file for
> "default" (or perhaps better name), where you have lowest-common denominator
> instruction-set and building for shared libraries?
> Would that work for everyone, or do people feel it would be too confusing to 
> have
> more defconfig files available?

Given the opposition to defaulting to shared, another config file seems 
like a fair compromise to me, whether "default" or something else. As 
for the naming, one possibility would be calling it "shared", implying 
both lowest-common denominator instruction set to be shareable across 
many systems and shared libraries.

- Panu -

[dpdk-dev] [PATCH] config: default to shared library

[David Marchand]

On Wed, Mar 4, 2015 at 2:49 PM, Bruce Richardson  wrote:

> On Wed, Mar 04, 2015 at 03:41:49PM +0200, Panu Matilainen wrote:
> > Right, but then there's "ivshmem" that doesn't fit that description
> either
> > AFAICS.
>
> Ah, yes, forgotten about that one! :-)


Well, this is out of scope, but this config file should not exist in the
first place.
>From my point of view, the ivshmem implementation is just badly hooked into
the eal, and this is the only reason why ivshmem should have a build option
(disabled) by default.

If we could cleanup this, then there would be no exception to the naming
convention.


-- 
David Marchand

[dpdk-dev] [PATCH] librte_lpm: define tbl entry reversely for big endian

[xuelin....@freescale.com]

From: Xuelin Shi 

This module uses type conversion between struct and int.
Also truncation and comparison is used with this int.
It is not safe for different endian arch.

Add ifdef for big endian struct to fix this issue.

Signed-off-by: Xuelin Shi 
---
 lib/librte_lpm/rte_lpm.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
index 1af150c..08a2859 100644
--- a/lib/librte_lpm/rte_lpm.h
+++ b/lib/librte_lpm/rte_lpm.h
@@ -96,6 +96,7 @@ extern "C" {
 /** Bitmask used to indicate successful lookup */
 #define RTE_LPM_LOOKUP_SUCCESS  0x0100

+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
 /** @internal Tbl24 entry structure. */
 struct rte_lpm_tbl24_entry {
/* Stores Next hop or group index (i.e. gindex)into tbl8. */
@@ -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
uint8_t valid_group :1; /**< Group validation flag. */
uint8_t depth   :6; /**< Rule depth. */
 };
+#else
+struct rte_lpm_tbl24_entry {
+   uint8_t depth   :6;
+   uint8_t ext_entry   :1;
+   uint8_t valid   :1;
+   union {
+   uint8_t tbl8_gindex;
+   uint8_t next_hop;
+   };
+};
+
+struct rte_lpm_tbl8_entry {
+   uint8_t depth   :6;
+   uint8_t valid_group :1;
+   uint8_t valid   :1;
+   uint8_t next_hop;
+};
+#endif

 /** @internal Rule structure. */
 struct rte_lpm_rule {
-- 
1.9.1

[dpdk-dev] [PATCH] config: default to shared library

[Bruce Richardson]

On Wed, Mar 04, 2015 at 02:57:50PM +0100, David Marchand wrote:
> On Wed, Mar 4, 2015 at 2:49 PM, Bruce Richardson  intel.com
> > wrote:
> 
> > On Wed, Mar 04, 2015 at 03:41:49PM +0200, Panu Matilainen wrote:
> > > Right, but then there's "ivshmem" that doesn't fit that description
> > either
> > > AFAICS.
> >
> > Ah, yes, forgotten about that one! :-)
> 
> 
> Well, this is out of scope, but this config file should not exist in the
> first place.
> From my point of view, the ivshmem implementation is just badly hooked into
> the eal, and this is the only reason why ivshmem should have a build option
> (disabled) by default.
> 
> If we could cleanup this, then there would be no exception to the naming
> convention.
> 
No objections here! :-)

> 
> -- 
> David Marchand

[dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Thomas Monjalon]

Hi Neil,

I remove parts that I agree and reply to those which deserve more discussion.

2015-03-04 06:49, Neil Horman:
> On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > 2015-02-02 13:18, Neil Horman:
> > > +# Validate that we have all the arguments we need
> > > +res=$(validate_args)
> > > +if [ -n "$res" ]
> > > +then
> > > + echo $res
> > 
> > Should be redirected to stderr >&2
> > 
> Why? this is eactly what I intended.  All the other messages from log are
> directed to stdout, so should this be.

I'm wondering if there's some normal output which could be redirected for
further processing, and some error output.
My comment was not only for this log but also for every error message.

> > I guess this is the tool:
> > http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> 
> Correct.

So maybe you should add this URL in the commit log.

> > > +log "INFO" "We're going to check and make sure that applications built"
> > > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when 
> > > executed"
> > > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > > +log "INFO" ""

> > > +if [ ! -d ./.git ]
> > > +then
> > > + log "WARN" "You must be in the root of the dpdk git tree"
> > > + log "WARN" "You are in $PWD"
> > > + cleanup_and_exit 1
> > > +fi
> > 
> > Why not cd $(dirname $0)/.. instead of returning an error?
> 
> Why would that help in finding the base of the git tree.  Theres no guarantee
> that you are in a subdirectory of a git tree.  I suppose we can try it
> recursively until we hit /, but it seems just as easy and clear to tell the 
> user
> whats needed.

No I'm saying that you could avoid this check by going into the right
directory from the beginning.
We know that the root dir is $(dirname $0)/.. because this script is in
scripts/ directory.

> > > +# Make sure we configure SHARED libraries
> > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > 
> > Why not tuning configuration after make config in .config file?
> > 
> Because this way we save a reconfig (from a developer viewpoint), you should 
> run
> make config again after changing configs, and so this way you save doing that.

No, you run make config once and update .config file. That's the recommended
way to configure DPDK.
defconfig files are default configurations and should stay read-only.

> > > +for i in `ls *.so`
> > 
> > I think ls is useless.
> > 
> Um, I don't?  Not sure what you're getting at here.

I think "for i in *.so" should work.

> > > + #compare the abi dumps
> > > + $ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new $ABI_DIR/$NEWNAME
> > 
> > Do we need to do a visual check? I didn't try yet.
> > 
> Yes, it generates an html report of all the symbols exported in a build and
> compares them with the alternate version.  That needs manual review.

OK I think it's important to explain in the commit log.

> > So you compare the ABI dumps.
> > Do we also need to run an app from TAG2 with libs from TAG1?
> 
> I started down that path, but its not really that helpful, as all it will do 
> is
> refuse to run if there is a symbol missing from a later version.  While that
> might be helpful, its no where near as through as the full report from the
> compliance checker.
> 
> The bottom line is that real ABI compliance requires a developer to be aware 
> of
> the changes going into the code and how they affect binary layout. A simple
> "does it still work" test isn't sufficient.

I hope we'll be able to integrate this kind of tool in an automated sanity
check in order to find obvious errors.

Thanks

[dpdk-dev] [PATCH] config: default to shared library

[Bruce Richardson]

On Wed, Mar 04, 2015 at 03:24:12PM +0200, Panu Matilainen wrote:
> On 03/04/2015 03:08 PM, Bruce Richardson wrote:
> >On Wed, Mar 04, 2015 at 06:28:05AM -0500, Neil Horman wrote:
> >>On Wed, Mar 04, 2015 at 01:05:07PM +0200, Panu Matilainen wrote:
> >>>On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
> Hi Panu,
> 
> 2015-03-04 08:17, Panu Matilainen:
> >With symbol versioning its vital that developers test their code in
> >shared library mode, otherwise we'll be playing "add the forgotten
> >symbol export" from here to eternity.
> 
> Yes we must improve the sanity checks.
> A lot of options must be tested (or removed) and not only shared libs.
> But the error you reported before (missing export of 
> rte_eth_dev_release_port)
> cannot be seen even with this patch.
> >>>
> >>>I know, I didn't say it would have directly caught it. It would've likely
> >>>been found earlier though, if nothing else then in testing of the new
> >>>librte_pmd_null which clearly nobody had tried in shared lib configuration.
> >>>
> >>This is accurate.  The default config is a tool, in the sense that it 
> >>leverages
> >>the implicit testing of any users who are experimenting with the DPDK.  Any
> >>users out there using the DPDK test/example applications would have realized
> >>something was amiss when the testpmd app refused to run with the null or 
> >>pcap
> >>pmd, since there was a missing symbol.  That "social fuzzing" has value, 
> >>but it
> >>only works if the defaults are carefully selected.  Currently, building for
> >>shared libraries exposes more existing bugs than static libraries, and so we
> >>should set that as our default so as to catch them.
> >>
> It means we need more tools.
> Though, default configuration is not a tool.
> >>>
> >>>Yes, default config is not a tool, its a recommendation of sorts both for
> >>>developers and users. It also tends to be the setup that is rarely broken
> >>>because it happens to get the most testing :)
> >>>
> >>And it is a tool (see above).
> >>
> 
> >By defaulting to shared we should catch more of these cases early,
> >but without taking away anybodys ability to build static.
> 
> Shared libraries are convenient for distributions but have a performance
> impact. I think that static build must remain the default choice.
> >>>
> >>
> >>If utmost performance is the concern, isn't it reasonable to assume that 
> >>users
> >>in that demographic will customize their configuration to achieve that?  No 
> >>one
> >>assumes that something is tuned to be perfect for their needs out of the 
> >>box if
> >>their needs are extreemely biased to a single quality.  The best course of
> >>action here is to set the default to be adventageous toward catching bugs, 
> >>and
> >>document the changes needed to bias for performance.
> >>
> >>>For distros, this is not a matter of *convenience*, its the only 
> >>>technically
> >>>feasible choice.
> >
> >As I understand it, build for the "default" cpu rather than "native" is the 
> >only
> >feasible choice also, so how about re-introducing a new defconfig file for
> >"default" (or perhaps better name), where you have lowest-common denominator
> >instruction-set and building for shared libraries?
> >Would that work for everyone, or do people feel it would be too confusing to 
> >have
> >more defconfig files available?
> 
> Given the opposition to defaulting to shared, another config file seems like
> a fair compromise to me, whether "default" or something else. As for the
> naming, one possibility would be calling it "shared", implying both
> lowest-common denominator instruction set to be shareable across many
> systems and shared libraries.
> 
>   - Panu -

The naming scheme for configs is meant to be:
"ARCH-MACHINE-EXECENV-TOOLCHAIN" 
as documented in the Getting Started Guide. "Default" has been used up till now
to refer to the lowest common denominator instruction set supported, which for
x86_64 is a core2 baseline, I believe. "shared" doesn't really fit into this
naming scheme, and there is nothing to allow extra notes to be added to the
name.
Without changing this scheme, I would suggest we rename "default" to "generic",
which I think is a slightly better term for it, and we set the
"x86_64-generic-linuxapp-gcc" target to build shared libs.

/Bruce

[dpdk-dev] [PATCH v3] pci: save list of detached devices, and re-probe during driver unload

[Raz Amir]

Added code that saves the pointers to the detached devices, during
driver loading, and during driver unloading, go over the list,
and re-attach them by calling device_probe_and_attach
on each device.

Signed-off-by: Raz Amir 
---
 lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 32 
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c 
b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
index 5ae8560..78e4dea 100644
--- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
+++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
@@ -55,6 +55,9 @@ __FBSDID("$FreeBSD$");

 #define MAX_BARS (PCIR_MAX_BAR_0 + 1)

+#define MAX_DETACHED_DEVICES   128
+static device_t detached_devices[MAX_DETACHED_DEVICES] = {};
+static int num_detached = 0;

 struct nic_uio_softc {
device_tdev_t;
@@ -289,16 +292,37 @@ nic_uio_load(void)

dev = pci_find_bsf(bus, device, function);
if (dev != NULL)
-   for (i = 0; i < NUM_DEVICES; i++)
-   if (pci_get_vendor(dev) == devices[i].vend &&
-   pci_get_device(dev) == 
devices[i].dev)
-   device_detach(dev);
+   continue;
+   
+   for (i = 0; i < NUM_DEVICES; i++)
+   if (pci_get_vendor(dev) == devices[i].vend &&
+   pci_get_device(dev) == devices[i].dev) {
+   if (num_detached < 
MAX_DETACHED_DEVICES) {
+   printf("nic_uio_load: 
detaching and storing dev=%p\n", dev);
+   
detached_devices[num_detached++] = dev;
+   }
+   else
+   printf("nic_uio_load: 
reached MAX_DETACHED_DEVICES=%d. dev=%p won't be reattached\n",
+   
MAX_DETACHED_DEVICES, dev);
+   device_detach(dev);
+   }
}
 }

 static void
 nic_uio_unload(void)
 {
+   int i;
+   printf("nic_uio_unload: entered ... \n");
+
+   for (i = 0; i < num_detached; i++) {
+   printf("nic_uio_unload: calling to device_probe_and_attach for 
dev=%p...\n",
+   detached_devices[i]);
+   device_probe_and_attach(detached_devices[i]);
+   printf("nic_uio_unload: done.\n");
+   }
+
+   printf("nic_uio_unload: leaving ... \n");
 }

 static int
-- 
2.1.2

[dpdk-dev] [PATCH] config: default to shared library

[Bruce Richardson]

On Wed, Mar 04, 2015 at 06:28:05AM -0500, Neil Horman wrote:
> On Wed, Mar 04, 2015 at 01:05:07PM +0200, Panu Matilainen wrote:
> > On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
> > >Hi Panu,
> > >
> > >2015-03-04 08:17, Panu Matilainen:
> > >>With symbol versioning its vital that developers test their code in
> > >>shared library mode, otherwise we'll be playing "add the forgotten
> > >>symbol export" from here to eternity.
> > >
> > >Yes we must improve the sanity checks.
> > >A lot of options must be tested (or removed) and not only shared libs.
> > >But the error you reported before (missing export of 
> > >rte_eth_dev_release_port)
> > >cannot be seen even with this patch.
> > 
> > I know, I didn't say it would have directly caught it. It would've likely
> > been found earlier though, if nothing else then in testing of the new
> > librte_pmd_null which clearly nobody had tried in shared lib configuration.
> > 
> This is accurate.  The default config is a tool, in the sense that it 
> leverages
> the implicit testing of any users who are experimenting with the DPDK.  Any
> users out there using the DPDK test/example applications would have realized
> something was amiss when the testpmd app refused to run with the null or pcap
> pmd, since there was a missing symbol.  That "social fuzzing" has value, but 
> it
> only works if the defaults are carefully selected.  Currently, building for
> shared libraries exposes more existing bugs than static libraries, and so we
> should set that as our default so as to catch them.
> 
> > >It means we need more tools.
> > >Though, default configuration is not a tool.
> > 
> > Yes, default config is not a tool, its a recommendation of sorts both for
> > developers and users. It also tends to be the setup that is rarely broken
> > because it happens to get the most testing :)
> > 
> And it is a tool (see above).
> 
> > >
> > >>By defaulting to shared we should catch more of these cases early,
> > >>but without taking away anybodys ability to build static.
> > >
> > >Shared libraries are convenient for distributions but have a performance
> > >impact. I think that static build must remain the default choice.
> > 
> 
> If utmost performance is the concern, isn't it reasonable to assume that users
> in that demographic will customize their configuration to achieve that?  No 
> one
> assumes that something is tuned to be perfect for their needs out of the box 
> if
> their needs are extreemely biased to a single quality.  The best course of
> action here is to set the default to be adventageous toward catching bugs, and
> document the changes needed to bias for performance.
> 
> > For distros, this is not a matter of *convenience*, its the only technically
> > feasible choice.

As I understand it, build for the "default" cpu rather than "native" is the only
feasible choice also, so how about re-introducing a new defconfig file for
"default" (or perhaps better name), where you have lowest-common denominator
instruction-set and building for shared libraries?
Would that work for everyone, or do people feel it would be too confusing to 
have
more defconfig files available?

/Bruce

> > 
> > I didn't want to make the commit message into a shared library sermon, but
> > if you look at the OSS landscape overall the common wisdom is that shared
> > library benefits outweigh any performance impact by so much that static libs
> > are almost nowhere to be found. I can change the text into a full-blown
> > rationale why shared libraries should be the default if that makes any
> > difference.
> > 
> Embedded applications actually do make extensive use of static linking to try
> achieve greater performance, but they tend to be proprietary, and as such are
> the exception that proves the rule.  Once an application itself becomes open
> source, it biases toward shared libraries, because the minor performance 
> impact
> is well worth the increased manageability and security found in DSO's
> 
> Acked-by: Neil Horman 
> 
> > - Panu -
> > 
> > 

[dpdk-dev] proposal: remove separate doc tree

[Thomas Monjalon]

2015-02-11 14:28, Butler, Siobhan A:
> Hi all,
> 
> In creating documentation for DPDK 1.8.0 dpdk.org used a separate repository 
> 'dpdk-doc' to work on the conversion of old documentation from PDF to .rst 
> with sphinx for maintainability and usability purposes.
> Now that the conversion has been complete and patches to the documentation 
> can be made, I propose that we no longer need the additional repo and it 
> should be removed.
> 
> Many Thanks,
> Siobhan Butler

OK, it's now removed.
Thanks

[dpdk-dev] [PATCH] config: default to shared library

[Panu Matilainen]

On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
> Hi Panu,
>
> 2015-03-04 08:17, Panu Matilainen:
>> With symbol versioning its vital that developers test their code in
>> shared library mode, otherwise we'll be playing "add the forgotten
>> symbol export" from here to eternity.
>
> Yes we must improve the sanity checks.
> A lot of options must be tested (or removed) and not only shared libs.
> But the error you reported before (missing export of rte_eth_dev_release_port)
> cannot be seen even with this patch.

I know, I didn't say it would have directly caught it. It would've 
likely been found earlier though, if nothing else then in testing of the 
new librte_pmd_null which clearly nobody had tried in shared lib 
configuration.

> It means we need more tools.
> Though, default configuration is not a tool.

Yes, default config is not a tool, its a recommendation of sorts both 
for developers and users. It also tends to be the setup that is rarely 
broken because it happens to get the most testing :)

>
>> By defaulting to shared we should catch more of these cases early,
>> but without taking away anybodys ability to build static.
>
> Shared libraries are convenient for distributions but have a performance
> impact. I think that static build must remain the default choice.

For distros, this is not a matter of *convenience*, its the only 
technically feasible choice.

I didn't want to make the commit message into a shared library sermon, 
but if you look at the OSS landscape overall the common wisdom is that 
shared library benefits outweigh any performance impact by so much that 
static libs are almost nowhere to be found. I can change the text into a 
full-blown rationale why shared libraries should be the default if that 
makes any difference.

- Panu -

[dpdk-dev] Guest Machine is not Pingable from Host Machine

[Arkajit Ghosh]

Hi Team,

Guest machine is not pingable from Host machine after creating a bridge with 
datapath_type "netdev" in the configuration database and adding  dpdk ports. 
Can anyone please let me know what is the issue.

Thanks in advance. 

Thanks & Regards
Arkajit Ghosh

=-=-=
Notice: The information contained in this e-mail
message and/or attachments to it may contain 
confidential or privileged information. If you are 
not the intended recipient, any dissemination, use, 
review, distribution, printing or copying of the 
information contained in this e-mail message 
and/or attachments to it are strictly prohibited. If 
you have received this communication in error, 
please notify us by reply e-mail or telephone and 
immediately and permanently delete the message 
and any attachments. Thank you

[dpdk-dev] [PATCH v11 2/2] librte_pmd_null: Support port hotplug function

[Tetsuya Mukawa]

2015-02-26 19:57 GMT+09:00 Thomas Monjalon :
> 2015-02-26 18:06, Tetsuya Mukawa:
>> 2015-02-26 16:03 GMT+09:00 Thomas Monjalon :
>> > 2015-02-25 16:49, Stephen Hemminger:
>> >> Build fails if HOTPLUG is disabled
>>
>> Hi Stephen,
>>
>> I appreciate for you reporting.
>>
>> >
>> > OK thanks for reporting.
>> > Actually there is no good reason to disable hotplug on Linux.
>> > Though it means that it's impossible to build on FreeBSD.
>> >
>> > Tetsuya, the right fix is to remove this option.
>>
>> Hi Thomas,
>>
>> Yes, I agree with it. I will add some codes to remove the option.
>> Please let me have a few days, I need to prepare BSD machine for compile 
>> test.
>>
>> > You should manage to graceful degrades hotplug in not supported
>> > cases supported: devices cannot be detachable in case of VFIO or nic_uio.
>> > What about uio_pci_generic?
>>
>> We cannot detach a vfio device so far. But we can detach a igb_uio and
>> uio_pci_generic device.
>> About a vfio ddevice, I haven't checked related code yet, but I guess
>> I will submit code to detach a vfio device in post-rc1.
>
> OK thanks.
> I made a quick fix (moving #ifdef) waiting the option removal:
> http://dpdk.org/browse/dpdk/commit/?id=7609e6609350
>

Hi Thomas,

I submit the patches to remove HOTPLUG config macro.

Thanks,
Tetsuya

[dpdk-dev] Build failure on FreeBSD-10.1-RELEASE

[Tetsuya Mukawa]

On 2015/03/02 19:22, Bruce Richardson wrote:
> On Mon, Mar 02, 2015 at 12:47:42PM +0900, Tetsuya Mukawa wrote:
>> Hi,
>>
>> I got a error while building master branch on FreeBSD.
>> Here is a log.
>>
>> $ gmake T=x86_64-native-bsdapp-clang config
>> cc: error: unknown argument: '-fdirectives-only'
>> cp: /usr/home/mukawa/work/dpdk/build/.config_tmp: No such file or directory
>> cp: /usr/home/mukawa/work/dpdk/build/.config_tmp: No such file or directory
>> gmake[3]: Nothing to be done for 'depdirs'.
>> Configuration done
>>
>>
>> Here is log came from 'uname'
>>
>> $ uname -a
>> FreeBSD eris.hq.igel.co.jp 10.1-RELEASE FreeBSD 10.1-RELEASE #0 r274401:
>> Tue Nov 11 21:02:49 UTC 2014
>>
>>
>> I've tried to remove '-fdirectives-only' from mk/rte.sdkconfig.mk like
>> below.
>> With the fixing,  It seems I can compile and run testpmd.
>> (Obviously, we should not merge below patch, but I've done just for testing)
>>
>> diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk
>> index d43c430..f8d95b1 100644
>> --- a/mk/rte.sdkconfig.mk
>> +++ b/mk/rte.sdkconfig.mk
>> @@ -75,7 +75,7 @@ else
>>  $(RTE_OUTPUT)/.config: $(RTE_CONFIG_TEMPLATE) FORCE | $(RTE_OUTPUT)
>> $(Q)if [ "$(RTE_CONFIG_TEMPLATE)" != "" -a -f
>> "$(RTE_CONFIG_TEMPLATE)" ]; then \
>> $(CPP) -undef -P -x assembler-with-cpp \
>> -   -fdirectives-only -ffreestanding \
>> +   -ffreestanding \
>> -o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
>> if ! cmp -s $(RTE_OUTPUT)/.config_tmp
>> $(RTE_OUTPUT)/.config; then \
>> cp $(RTE_OUTPUT)/.config_tmp
>> $(RTE_OUTPUT)/.config ; \
>>
>>
>> Also, I've checked /usr/ports/net/dpdk, and found below line.
>> (It seems above ports dpdk package is based on DPDK-1.8.)
>>
>>
>> $(CPP) -undef -P -x assembler-with-cpp \
>>  -ffreestanding \
>>-o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
>>
>> So, I guess we should not add '-fdirectives-only' for flags of $(CPP)
>> for BSD system like dpdk package of ports.
>>
>> Thanks,
>> Tetsuya
>>
> Yes, that is correct. In most cases I have tested, the extra flag only gives a
> warning but it appears its now an error. We should conditionally include or
> omit the flag for BSD vs Linux, I think.
>
> /Bruce
Hi Bruce,

It seems we cannot use CONFIG_RTE_EXEC_ENV_LINUXAPP/BSDAPP definition here.
Now I am looking for other way to check target OS.
Is it not so good to use $(T) definition value here?

Thanks,
Tetsuya

[dpdk-dev] [PATCH] config: default to shared library

[Thomas Monjalon]

2015-03-04 13:05, Panu Matilainen:
> On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
> > Hi Panu,
> >
> > 2015-03-04 08:17, Panu Matilainen:
> >> With symbol versioning its vital that developers test their code in
> >> shared library mode, otherwise we'll be playing "add the forgotten
> >> symbol export" from here to eternity.
> >
> > Yes we must improve the sanity checks.
> > A lot of options must be tested (or removed) and not only shared libs.
> > But the error you reported before (missing export of 
> > rte_eth_dev_release_port)
> > cannot be seen even with this patch.
> 
> I know, I didn't say it would have directly caught it. It would've 
> likely been found earlier though, if nothing else then in testing of the 
> new librte_pmd_null which clearly nobody had tried in shared lib 
> configuration.

Exactly, there was no test in shared lib mode.
Then I recommend having a basic testpmd trial which should be easy with
PMD null.
Any script to ease this test is welcome.

> > It means we need more tools.
> > Though, default configuration is not a tool.
> 
> Yes, default config is not a tool, its a recommendation of sorts both 
> for developers and users. It also tends to be the setup that is rarely 
> broken because it happens to get the most testing :)
> 
> >
> >> By defaulting to shared we should catch more of these cases early,
> >> but without taking away anybodys ability to build static.
> >
> > Shared libraries are convenient for distributions but have a performance
> > impact. I think that static build must remain the default choice.
> 
> For distros, this is not a matter of *convenience*, its the only 
> technically feasible choice.

"only" is a religious word here :) We could imagine an automatic rebuild
of DPDK applications when DPDK is updated. But it's not the topic :)

> I didn't want to make the commit message into a shared library sermon, 
> but if you look at the OSS landscape overall the common wisdom is that 
> shared library benefits outweigh any performance impact by so much that 
> static libs are almost nowhere to be found. I can change the text into a 
> full-blown rationale why shared libraries should be the default if that 
> makes any difference.

No, as Bruce agreed, it's better to keep static as default, because DPDK is
performance-oriented.
I agree we really have a problem with tests and I'd prefer we solve it by
adding some scripts. I'm currently thinking about which scripts to add,
like building different configurations. Then the second step will be to
automate the launch of these scripts to catch wrong submissions earlier.
Maybe that something like patchew (http://qemu.patchew.org) could help for
the second step (thanks David for the tip).

[dpdk-dev] [PATCH] pci: save list of detached devices, and re-probe during driver unload

[Raz Amir]

Understood.
I already sent the updated patch, so I will fix this and resend it soon.

-Original Message-
From: Bruce Richardson [mailto:bruce.richard...@intel.com] 
Sent: 04 March 2015 12:13
To: Raz Amir
Cc: dev at dpdk.org
Subject: Re: [dpdk-dev] [PATCH] pci: save list of detached devices, and
re-probe during driver unload

On Wed, Mar 04, 2015 at 11:07:41AM +0200, Raz Amir wrote:
> Thank you.
> 
> See answers inline (mostly ack, but not only), and I will send the 
> updated patch soon.
> 
>  
> 
> > -Original Message-
> 
> > From: Bruce Richardson [mailto:bruce.richardson at intel.com]
> 
> > Sent: 03 March 2015 15:33
> 
> > To: Raz Amir
> 
> > Cc: dev at dpdk.org
> 
> > Subject: Re: [dpdk-dev] [PATCH] pci: save list of detached devices, 
> > and
> re-
> 
> > probe during driver unload
> 
> > 
> 
> > On Thu, Feb 26, 2015 at 06:33:20AM +, Raz Amir wrote:
> 
> > > Added code that saves the pointers to the detached devices, during
> 
> > > driver loading, and during driver unloading, go over the list, and
> 
> > > re-attach them by calling device_probe_and_attach on each device.
> 
> > >
> 
> > > Signed-off-by: Raz Amir < 
> razamir22 at gmail.com>
> 
> > 
> 
> > Couple of minor comments below. Otherwise all looks good to me.
> 
> > 
> 
> > Acked-by: Bruce Richardson < 
> bruce.richardson at intel.com>
> 
> > > ---
> 
> > >  lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 26
> 
> > > +-
> 
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> > >
> 
> > > diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > index 5ae8560..7d702a5 100644
> 
> > > --- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > +++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > @@ -55,6 +55,9 @@ __FBSDID("$FreeBSD$");
> 
> > >
> 
> > >  #define MAX_BARS (PCIR_MAX_BAR_0 + 1)
> 
> > >
> 
> > > +#define MAX_DETACHED_DEVICES   128
> 
> > > +static device_t detached_devices[MAX_DETACHED_DEVICES] = {}; 
> > > +static
> 
> > > +int last_detached = 0;
> 
> > Maybe num_detached/nb_detached or even just "detached" instead of
> 
> > "last_detached".
> 
> Ack.
> 
>  
> 
> > 
> 
> > >
> 
> > >  struct nic_uio_softc {
> 
> > >  device_tdev_t;
> 
> > > @@ -291,14 +294,35 @@ nic_uio_load(void)
> 
> > >  if (dev != NULL)
> 
> > 
> 
> > We are getting into some serious levels of indentation below, so 
> > maybe
> flip
> 
> > this condition around and put in a "continue" instead, so that we 
> > can
> dedent
> 
> > everything below that follows it.
> 
> > 
> 
> Ack.
> 
>  
> 
> > >  for (i = 0; i < 
> > > NUM_DEVICES;
> i++)
> 
> > >  if
> (pci_get_vendor(dev) == devices[i].vend
> 
> > &&
> 
> > > -
> pci_get_device(dev) ==
> 
> > devices[i].dev)
> 
> > > +
> pci_get_device(dev) ==
> 
> > devices[i].dev) {
> 
> > > +
> if (last_detached+1 <
> 
> > MAX_DETACHED_DEVICES) {
> 
> > I don't think you need the +1 here.
> 
> It is needed, otherwise the last object will be added at 
> MAX_DETACHED_DEVICES position while the last position is 
> MAX_DETACHED_DEVICES-1.

Yes, the last position is MAX_DETACHED_DEVICES-1, but you do the addition of
the element to the array using "detached_devices[last_detached++]", i.e. a
post-increment, so when last_detached == (MAX_DETACHED_DEVICES-1), you still
can fill in an entry. Next time around, when last_detached ==
MAX_DETACHED_DEVICES it's no longer safe to add, and the condition
"last_detached < MAX_DETACHED_DEVICES) will now fail. No +1 or -1 necessary
to prevent this.

/Bruce

[dpdk-dev] [PATCH 3/3] doc: Remove BSD limitation from hotplug section of programmer's guide

[Tetsuya Mukawa]

This patch removes below limitation from hotplug section of
programmer's guide.

- The framework can only be enabled with Linux. BSD is not supported.

Signed-off-by: Tetsuya Mukawa 
---
 doc/guides/prog_guide/port_hotplug_framework.rst | 2 --
 1 file changed, 2 deletions(-)

diff --git a/doc/guides/prog_guide/port_hotplug_framework.rst 
b/doc/guides/prog_guide/port_hotplug_framework.rst
index fe6d72a..17030bf 100644
--- a/doc/guides/prog_guide/port_hotplug_framework.rst
+++ b/doc/guides/prog_guide/port_hotplug_framework.rst
@@ -99,8 +99,6 @@ Limitations

 *   The Port Hotplug APIs are not thread safe.

-*   The framework can only be enabled with Linux. BSD is not supported.
-
 *   To detach a port, the port should be backed by a device that igb_uio
 manages. VFIO is not supported.

-- 
1.9.1

[dpdk-dev] [PATCH 2/3] eal, ethdev: Remove CONFIG_RTE_LIBRTE_EAL_HOTPLUG

[Tetsuya Mukawa]

The patch removes CONFIG_RTE_LIBRTE_EAL_HOTPLUG option from DPDK.

Signed-off-by: Tetsuya Mukawa 
---
 config/common_bsdapp   |  6 --
 config/common_linuxapp |  5 -
 lib/librte_eal/common/eal_common_dev.c |  2 --
 lib/librte_eal/common/eal_common_pci.c |  2 --
 lib/librte_eal/common/include/rte_pci.h|  2 --
 lib/librte_eal/linuxapp/eal/eal_pci.c  | 12 
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |  2 --
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c  |  2 --
 lib/librte_ether/rte_ethdev.c  | 21 -
 9 files changed, 54 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index 8ff4dc2..88b44e9 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -116,12 +116,6 @@ CONFIG_RTE_LIBRTE_EAL_BSDAPP=y
 CONFIG_RTE_LIBRTE_EAL_LINUXAPP=n

 #
-# Compile Environment Abstraction Layer to support hotplug
-# So far, Hotplug functions only support linux
-#
-CONFIG_RTE_LIBRTE_EAL_HOTPLUG=n
-
-#
 # Compile Environment Abstraction Layer to support Vmware TSC map
 #
 CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=y
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 97f1c9e..f9c9780 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -114,11 +114,6 @@ CONFIG_RTE_PCI_MAX_READ_REQUEST_SIZE=0
 CONFIG_RTE_LIBRTE_EAL_LINUXAPP=y

 #
-# Compile Environment Abstraction Layer to support hotplug
-#
-CONFIG_RTE_LIBRTE_EAL_HOTPLUG=y
-
-#
 # Compile Environment Abstraction Layer to support Vmware TSC map
 #
 CONFIG_RTE_LIBRTE_EAL_VMWARE_TSC_MAP_SUPPORT=y
diff --git a/lib/librte_eal/common/eal_common_dev.c 
b/lib/librte_eal/common/eal_common_dev.c
index 92a5a94..4089d66 100644
--- a/lib/librte_eal/common/eal_common_dev.c
+++ b/lib/librte_eal/common/eal_common_dev.c
@@ -125,7 +125,6 @@ rte_eal_dev_init(void)
return 0;
 }

-#ifdef RTE_LIBRTE_EAL_HOTPLUG
 int
 rte_eal_vdev_uninit(const char *name)
 {
@@ -151,4 +150,3 @@ rte_eal_vdev_uninit(const char *name)
RTE_LOG(ERR, EAL, "no driver found for %s\n", name);
return -EINVAL;
 }
-#endif /* RTE_LIBRTE_EAL_HOTPLUG */
diff --git a/lib/librte_eal/common/eal_common_pci.c 
b/lib/librte_eal/common/eal_common_pci.c
index 5b6b55d..80f51d3 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -126,7 +126,6 @@ pci_probe_all_drivers(struct rte_pci_device *dev)
return 1;
 }

-#ifdef RTE_LIBRTE_EAL_HOTPLUG
 /*
  * If vendor/device ID match, call the devuninit() function of all
  * registered driver for the given device. Return -1 if initialization
@@ -217,7 +216,6 @@ err_return:
dev->addr.devid, dev->addr.function);
return -1;
 }
-#endif /* RTE_LIBRTE_EAL_HOTPLUG */

 /*
  * Scan the content of the PCI bus, and call the devinit() function for
diff --git a/lib/librte_eal/common/include/rte_pci.h 
b/lib/librte_eal/common/include/rte_pci.h
index dc74821..3c8a26b 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -335,7 +335,6 @@ int rte_eal_pci_scan(void);
  */
 int rte_eal_pci_probe(void);

-#ifdef RTE_LIBRTE_EAL_HOTPLUG
 /**
  * Probe the single PCI device.
  *
@@ -365,7 +364,6 @@ int rte_eal_pci_probe_one(struct rte_pci_addr *addr);
  *   - Negative on error.
  */
 int rte_eal_pci_close_one(struct rte_pci_addr *addr);
-#endif /* RTE_LIBRTE_EAL_HOTPLUG */

 /**
  * Dump the content of the PCI bus.
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c 
b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 6d4932d..3914f52 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -595,7 +595,6 @@ pci_map_device(struct rte_pci_device *dev)
return ret;
 }

-#ifdef RTE_LIBRTE_EAL_HOTPLUG
 static void
 pci_unmap_device(struct rte_pci_device *dev)
 {
@@ -618,7 +617,6 @@ pci_unmap_device(struct rte_pci_device *dev)
break;
}
 }
-#endif /* RTE_LIBRTE_EAL_HOTPLUG */

 /*
  * If vendor/device ID match, call the devinit() function of the
@@ -691,7 +689,6 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, 
struct rte_pci_device *d
return 1;
 }

-#ifdef RTE_LIBRTE_EAL_HOTPLUG
 /*
  * If vendor/device ID match, call the devuninit() function of the
  * driver.
@@ -750,15 +747,6 @@ rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
/* return positive value if driver is not found */
return 1;
 }
-#else /* RTE_LIBRTE_EAL_HOTPLUG */
-int
-rte_eal_pci_close_one_driver(struct rte_pci_driver *dr __rte_unused,
-   struct rte_pci_device *dev __rte_unused)
-{
-   RTE_LOG(ERR, EAL, "Hotplug support isn't enabled\n");
-   return -1;
-}
-#endif /* RTE_LIBRTE_EAL_HOTPLUG */

 /* Init the PCI EAL subsystem */
 int
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_init.h 
b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
index 6af84d1..86088bd 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_init.h
+++ 

[dpdk-dev] [PATCH 1/3] BSD: Support Port Hotplug function

[Tetsuya Mukawa]

This patch adds Hotplug support to BSD.

Signed-off-by: Tetsuya Mukawa 
---
 lib/librte_eal/bsdapp/eal/eal_pci.c   | 169 +-
 lib/librte_eal/bsdapp/eal/rte_eal_version.map |   6 +
 lib/librte_eal/common/include/rte_pci.h   |   1 +
 lib/librte_ether/rte_ethdev.c |   1 +
 4 files changed, 174 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_pci.c 
b/lib/librte_eal/bsdapp/eal/eal_pci.c
index 9193f80..fc5a088 100644
--- a/lib/librte_eal/bsdapp/eal/eal_pci.c
+++ b/lib/librte_eal/bsdapp/eal/eal_pci.c
@@ -156,6 +156,26 @@ fail:
return NULL;
 }

+/* unmap a particular resource */
+static int
+pci_unmap_resource(void *requested_addr, size_t size)
+{
+   if (requested_addr == NULL)
+   return -EINVAL;
+
+   /* Unmap the PCI memory resource of device */
+   if (munmap(requested_addr, size)) {
+   RTE_LOG(ERR, EAL, "%s(): cannot munmap(%p, 0x%lx): %s\n",
+   __func__, requested_addr, (unsigned long)size,
+   strerror(errno));
+   } else {
+   RTE_LOG(DEBUG, EAL, "  PCI memory unmapped at %p\n",
+   requested_addr);
+   return -EFAULT;
+   }
+   return 0;
+}
+
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
@@ -270,6 +290,76 @@ pci_uio_map_resource(struct rte_pci_device *dev)
return (0);
 }

+static int
+pci_uio_unmap(struct uio_resource *uio_res)
+{
+   int ret;
+   unsigned i;
+
+   if (uio_res == NULL)
+   return -EINVAL;
+
+   for (i = 0; i != uio_res->nb_maps; i++) {
+   ret = pci_unmap_resource(uio_res->maps[i].addr,
+   (size_t)uio_res->maps[i].size);
+   if (ret < 0)
+   return ret;
+   }
+   return 0;
+}
+
+static struct uio_resource *
+pci_uio_find_resource(struct rte_pci_device *dev)
+{
+   struct uio_resource *uio_res;
+
+   if (dev == NULL)
+   return NULL;
+
+   TAILQ_FOREACH(uio_res, uio_res_list, next) {
+
+   /* skip this element if it doesn't match our PCI address */
+   if (!rte_eal_compare_pci_addr(_res->pci_addr, >addr))
+   return uio_res;
+   }
+   return NULL;
+}
+
+/* map the PCI resource of a PCI device in virtual memory */
+static int
+pci_uio_unmap_resource(struct rte_pci_device *dev)
+{
+   struct uio_resource *uio_res;
+
+   if (dev == NULL)
+   return -EINVAL;
+
+   /* find an entry for the device */
+   uio_res = pci_uio_find_resource(dev);
+   if (uio_res == NULL)
+   return -ENODEV;
+
+   /* secondary processes - just free maps */
+   if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+   return pci_uio_unmap(uio_res);
+
+   TAILQ_REMOVE(uio_res_list, uio_res, next);
+
+   /* unmap all resources */
+   pci_uio_unmap(uio_res);
+
+   /* free_uio resource */
+   rte_free(uio_res);
+
+   /* close fd if in primary process */
+   close(dev->intr_handle.fd);
+
+   dev->intr_handle.fd = -1;
+   dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
+
+   return 0;
+}
+
 /* Scan one pci sysfs entry, and fill the devices list from it. */
 static int
 pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
@@ -307,6 +397,9 @@ pci_scan_one(int dev_pci_fd, struct pci_conf *conf)
/* FreeBSD has no NUMA support (yet) */
dev->numa_node = 0;

+   /* FreeBSD has only one pass through driver */
+   dev->pt_driver = RTE_PT_NIC_UIO;
+
 /* parse resources */
switch (conf->pc_hdr & PCIM_HDRTYPE) {
case PCIM_HDRTYPE_NORMAL:
@@ -376,8 +469,8 @@ skipdev:
  * Scan the content of the PCI bus, and add the devices in the devices
  * list. Call pci_scan_one() for each pci entry found.
  */
-static int
-pci_scan(void)
+int
+rte_eal_pci_scan(void)
 {
int fd = -1;
unsigned dev_count = 0;
@@ -487,6 +580,76 @@ rte_eal_pci_probe_one_driver(struct rte_pci_driver *dr, 
struct rte_pci_device *d
return 1;
 }

+/*
+ * If vendor/device ID match, call the devuninit() function of the
+ * driver.
+ */
+int
+rte_eal_pci_close_one_driver(struct rte_pci_driver *dr,
+   struct rte_pci_device *dev)
+{
+   struct rte_pci_id *id_table;
+   int ret;
+
+   if ((dr == NULL) || (dev == NULL))
+   return -EINVAL;
+
+   for (id_table = dr->id_table ; id_table->vendor_id != 0; id_table++) {
+
+   /* check if device's identifiers match the driver's ones */
+   if (id_table->vendor_id != dev->id.vendor_id &&
+   id_table->vendor_id != PCI_ANY_ID)
+   continue;
+   if (id_table->device_id != dev->id.device_id &&
+   id_table->device_id != PCI_ANY_ID)
+   continue;
+   if 

[dpdk-dev] [PATCH] lib/librte_vhost: remove vhost device from data plane when receive VHOST_SET_MEM_TABLE message

[Long, Thomas]

Acked-by: Tommy Long  


-Original Message-
From: Xie, Huawei 
Sent: Tuesday, March 3, 2015 2:26 AM
To: dev at dpdk.org
Cc: haifeng.lin at huawei.com; mukawa at igel.co.jp; Long, Thomas; Xie, Huawei
Subject: [PATCH] lib/librte_vhost: remove vhost device from data plane when 
receive VHOST_SET_MEM_TABLE message

This patch fixes the segfault issue in the case vhost receives new 
VHOST_SET_MEM_TABLE message without VHOST_VRING_GET_VRING_BASE(which we uses as 
the stop message).

Signed-off-by: Huawei Xie 
---
 lib/librte_vhost/vhost_user/virtio-net-user.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c 
b/lib/librte_vhost/vhost_user/virtio-net-user.c
index 97c5177..aa08706 100644
--- a/lib/librte_vhost/vhost_user/virtio-net-user.c
+++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
@@ -109,6 +109,10 @@ user_set_mem_table(struct vhost_device_ctx ctx, struct 
VhostUserMsg *pmsg)
if (dev == NULL)
return -1;

+   /* Remove from the data plane. */
+   if (dev->flags & VIRTIO_DEV_RUNNING)
+   notify_ops->destroy_device(dev);
+
if (dev->mem) {
free_mem_region(dev);
free(dev->mem);
-- 
1.8.1.4

[dpdk-dev] [PATCH v2] pci: save list of detached devices, and re-probe during driver unload

[Raz Amir]

Added code that saves the pointers to the detached devices, during
driver loading, and during driver unloading, go over the list,
and re-attach them by calling device_probe_and_attach
on each device.

Signed-off-by: Raz Amir 
---
 lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 32 
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c 
b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
index 5ae8560..077bdb3 100644
--- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
+++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
@@ -55,6 +55,9 @@ __FBSDID("$FreeBSD$");

 #define MAX_BARS (PCIR_MAX_BAR_0 + 1)

+#define MAX_DETACHED_DEVICES   128
+static device_t detached_devices[MAX_DETACHED_DEVICES] = {};
+static int num_detached = 0;

 struct nic_uio_softc {
device_tdev_t;
@@ -289,16 +292,37 @@ nic_uio_load(void)

dev = pci_find_bsf(bus, device, function);
if (dev != NULL)
-   for (i = 0; i < NUM_DEVICES; i++)
-   if (pci_get_vendor(dev) == devices[i].vend &&
-   pci_get_device(dev) == 
devices[i].dev)
-   device_detach(dev);
+   continue;
+   
+   for (i = 0; i < NUM_DEVICES; i++)
+   if (pci_get_vendor(dev) == devices[i].vend &&
+   pci_get_device(dev) == devices[i].dev) {
+   if (num_detached < 
MAX_DETACHED_DEVICES-1) {
+   printf("nic_uio_load: 
detaching and storing dev=%p\n", dev);
+   
detached_devices[num_detached++] = dev;
+   }
+   else
+   printf("nic_uio_load: 
reached MAX_DETACHED_DEVICES=%d. dev=%p won't be reattached\n",
+   
MAX_DETACHED_DEVICES, dev);
+   device_detach(dev);
+   }
}
 }

 static void
 nic_uio_unload(void)
 {
+   int i;
+   printf("nic_uio_unload: entered ... \n");
+
+   for (i = 0; i < num_detached; i++) {
+   printf("nic_uio_unload: calling to device_probe_and_attach for 
dev=%p...\n",
+   detached_devices[i]);
+   device_probe_and_attach(detached_devices[i]);
+   printf("nic_uio_unload: done.\n");
+   }
+
+   printf("nic_uio_unload: leaving ... \n");
 }

 static int
-- 
2.1.2

[dpdk-dev] [PATCH v2] tools/dpdk_nic_bind: Fix can't bind virtio-pci issue

[Qiu, Michael]

On 3/4/2015 10:56 AM, Ouyang Changchun wrote:
> The dpdk_nic_bind script will not allow ports to be bound or unbound if none 
> of the
> kernel modules supported by DPDK is loaded. This patch relaxes this 
> restriction by
> checking if a DPDK module is actually requested. The example below 
> illustrates this
> problem:
>
> In virtio test, on the guest
> 1. Bind virtio port to igb_uio driver;
> 2. Remove igb_uio module;
> 3. Bind virtio port to virtio-pci driver, it fails and reports:
>"Error - no supported modules are loaded"
>
> The script should check the to-be-bound driver flag, if it is dpdk 
> driver(igb_uio, vfio etc),
> and the corresponding module is not loaded, then exit, otherwise, just report 
> a warning,
> and continue to bind the non-dpdk driver(like virtio-pci) to dev.
>
> Signed-off-by: Changchun Ouyang 
> ---
>
> Changes in v2:
>   -- Update the description.
>
>  tools/dpdk_nic_bind.py | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/dpdk_nic_bind.py b/tools/dpdk_nic_bind.py
> index 2483056..8523f82 100755
> --- a/tools/dpdk_nic_bind.py
> +++ b/tools/dpdk_nic_bind.py
> @@ -175,8 +175,11 @@ def check_modules():
>  
>  # check if we have at least one loaded module
>  if True not in [mod["Found"] for mod in mods] and b_flag is not None:
> -print "Error - no supported modules are loaded"
> -sys.exit(1)
> +if b_flag in dpdk_drivers:
> +print "Error - no supported modules(DPDK driver) are loaded"
> +sys.exit(1)
> +else:
> +print "Warning - no supported modules(DPDK driver) are loaded"
>  
>  # change DPDK driver list to only contain drivers that are loaded
>  dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
Acked-by: Michael Qiu 

[dpdk-dev] [PATCH v2 0/4] Fix issues reported by static analysis tool

[Thomas Monjalon]

> > Static analysis report some issues against current DPDK version. Most of
> > them need only cosmetic code changes (changing type of variable).
> > 
> > One issue related with ring pmd fix real memory leak problem.
> > 
> > PATCH v2 changes:
> >  - remove patch 5/5 as it was NACKed
> >  - reword commit log acording to mailing list sugestions.
> > 
> > Pawel Wodkowski (4):
> >   rte_timer: change declaration of rte_timer_cb_t
> >   librte_kvargs: make rte_kvargs_free() be consistent with other
> > "free()" functions
> >   pmd ring: fix possible memory leak during devinit
> >   cmdline: make parse_set_list() use size_t instead of int for low/high 
> >parameter
> 
> Series:
> Acked-by: Olivier Matz 

Applied, thanks

[dpdk-dev] [PATCH v3] ABI: Add abi checking utility

[Neil Horman]

There was a request for an abi validation utilty for the ongoing ABI stability
work.  As it turns out there is a abi compliance checker in development that
seems to be under active development and provides fairly detailed ABI compliance
reports.  Its not yet intellegent enough to understand symbol versioning, but it
does provide the ability to identify symbols which have changed between
releases, along with details of the change, and offers developers the
opportunity to identify which symbols then need versioning and validation for a
given update via manual testing.

This script automates the use of the compliance checker between two arbitrarily
specified tags within the dpdk tree.  To execute enter the $RTE_SDK directory
and run:

./scripts/validate_abi.sh $GIT_TAG1 $GIT_TAG2 $CONFIG

where $GIT_TAG1 and 2 are git tags and $CONFIG is a config specification
suitable for passing as the T= variable in the make config command.

Note the upstream source for the abi compliance checker is here:
http://ispras.linuxbase.org/index.php/ABI_compliance_checker

It generates a report for each DSO built from the requested tags that developers
can review to find ABI compliance issues.

Signed-off-by: Neil Horman 

---

Change Notes:

v2) Fixed some typos as requested by Thomas

v3) Fixed some additional typos Thomas requested
Improved script to work from detached state
Added some documentation to the changelog
Added some comments to the scripts
---
 scripts/validate_abi.sh | 248 
 1 file changed, 248 insertions(+)
 create mode 100755 scripts/validate_abi.sh

diff --git a/scripts/validate_abi.sh b/scripts/validate_abi.sh
new file mode 100755
index 000..899cf5f
--- /dev/null
+++ b/scripts/validate_abi.sh
@@ -0,0 +1,248 @@
+#!/bin/sh
+#   BSD LICENSE
+#
+#   Copyright(c) 2015 Neil Horman. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+# * Redistributions of source code must retain the above copyright
+#   notice, this list of conditions and the following disclaimer.
+# * Redistributions in binary form must reproduce the above copyright
+#   notice, this list of conditions and the following disclaimer in
+#   the documentation and/or other materials provided with the
+#   distribution.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+TAG1=$1
+TAG2=$2
+TARGET=$3
+ABI_DIR=`mktemp -d -p /tmp ABI.XX`
+
+usage() {
+   echo "$0   "
+}
+
+log() {
+   local level=$1
+   shift
+   echo "$*"
+}
+
+validate_tags() {
+   git tag -l | grep -q "$TAG1"
+   if [ $? -ne 0 ]
+   then
+   echo "$TAG1 is invalid"
+   return
+   fi
+   git tag -l | grep -q "$TAG2"
+   if [ $? -ne 0 ]
+   then
+   echo "$TAG2 is invalid"
+   return
+   fi
+}
+
+validate_args() {
+   if [ -z "$TAG1" ]
+   then
+   echo "Must Specify TAG1"
+   return
+   fi
+   if [ -z "$TAG2" ]
+   then
+   echo "Must Specify TAG2"
+   return
+   fi
+   if [ -z "$TARGET" ]
+   then
+   echo "Must Specify a build target"
+   fi
+}
+
+
+cleanup_and_exit() {
+   rm -rf $ABI_DIR
+   exit $1
+}
+
+###
+#START
+
+
+#trap on ctrl-c to clean up
+trap cleanup_and_exit SIGINT
+
+#Save the current branch
+CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
+
+if [ -z "$CURRENT_BRANCH" ]
+then
+   CURRENT_BRANCH=`git log --pretty=format:%H HEAD~1..HEAD`
+fi
+
+if [ -n "$VERBOSE" ]
+then
+   export VERBOSE=/dev/stdout
+else
+   export VERBOSE=/dev/null
+fi
+
+# Validate that we have all the arguments we need
+res=$(validate_args)
+if [ -n "$res" ]
+then
+   echo $res
+   usage
+   cleanup_and_exit 1
+fi
+
+# Make sure our tags exist
+res=$(validate_tags)
+if [ -n "$res" ]
+then
+   echo $res
+   cleanup_and_exit 1
+fi
+
+ABICHECK=`which abi-compliance-checker 2>/dev/null`
+if 

[dpdk-dev] [PATCH 0/7] fix build with debug enabled

[Thomas Monjalon]

2015-03-03 16:23, Thomas Monjalon:
> There are some compilation errors when debug options are enabled.
> We should start thinking to test every patch with an all-yes configuration.
> Unfortunately, we cannot force this kind of configuration because
> some libraries depend on the availability of some libraries.
> 
> Thomas Monjalon (7):
>   mempool: fix build with debug enabled
>   fm10k: fix build with debug enabled
>   virtio: fix build with mempool debug enabled
>   virtio: fix build with debug enabled
>   mlx4: fix build with mempool debug enabled
>   mlx4: mute auto config in quiet mode
>   bond: remove debug function to fix link with shared lib

Got acknowledgement from almost all concerned maintainers, thanks.
Applied

[dpdk-dev] [PATCH v6 0/8] Interrupt mode PMD

[Liang, Cunming]

Hi Stephen,

On 3/4/2015 8:52 AM, Stephen Hemminger wrote:
> On Fri, 27 Feb 2015 11:38:25 +0100
> David Marchand  wrote:
>
>> Ok, so after looking at this patchset, I would say this is the right 
>> direction, but still this is too limited.
>> The ethdev part and the vfio eventfds part look acceptable to me.
>> But thinking about it, I could just reuse a standard event library with the 
>> eventfds I would get from ethdev without a need for a new eal api.
> I would prefer that there was just an fd and a callback.
> An application should be able to use what ever event model or library it 
> wants.
[LCM] I agree, on application perspective it is.
As it's easy to get RX/TX interrupt fd, there's no limit for application 
to do all the things with the 3rd party event library.
The improvement probably be 1) a rte_intr_vec_to_fd() API; 2) expose 
eal_intr_process_rxtx_interrupts() as a public API for RX/TX interrupt 
callback.
However, it should allow to use the packet interrupt feature in case 
application don't choose any 3rd party event library.
That's the motivation to give a very lightweight 'wait' EAL API. Sounds 
reasonable ?
>
> IMHO the existing interrupt thread model is incorrectly designed and creates
> lots of opportunities for races because of that. Look at the effort it has to
> use to pass the event back to link state code.

[dpdk-dev] [PATCH] Move mk/rte.extvars.mk to mk/internal/rte.extvars.mk

[Keith Wiles]

Move the rte.extvars.mk to an internal directory and
update rte.vars.mk to find the file in the new location.

Signed-off-by: Keith Wiles 
---
 mk/internal/rte.extvars.mk | 81 ++
 mk/rte.extvars.mk  | 81 --
 mk/rte.vars.mk |  4 +--
 3 files changed, 83 insertions(+), 83 deletions(-)
 create mode 100644 mk/internal/rte.extvars.mk
 delete mode 100644 mk/rte.extvars.mk

diff --git a/mk/internal/rte.extvars.mk b/mk/internal/rte.extvars.mk
new file mode 100644
index 000..3e5a990
--- /dev/null
+++ b/mk/internal/rte.extvars.mk
@@ -0,0 +1,81 @@
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+# * Redistributions of source code must retain the above copyright
+#   notice, this list of conditions and the following disclaimer.
+# * Redistributions in binary form must reproduce the above copyright
+#   notice, this list of conditions and the following disclaimer in
+#   the documentation and/or other materials provided with the
+#   distribution.
+# * Neither the name of Intel Corporation nor the names of its
+#   contributors may be used to endorse or promote products derived
+#   from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+#
+# directory where sources are located
+#
+ifdef S
+ifeq ("$(origin S)", "command line")
+RTE_SRCDIR := $(abspath $(S))
+endif
+endif
+RTE_SRCDIR  ?= $(CURDIR)
+export RTE_SRCDIR
+
+#
+# Makefile to call once $(RTE_OUTPUT) is created
+#
+ifdef M
+ifeq ("$(origin M)", "command line")
+RTE_EXTMK := $(abspath $(M))
+endif
+endif
+RTE_EXTMK ?= $(RTE_SRCDIR)/Makefile
+export RTE_EXTMK
+
+RTE_SDK_BIN := $(RTE_SDK)/$(RTE_TARGET)
+
+#
+# Output files wil go in a separate directory: default output is
+# $(RTE_SRCDIR)/build
+# Output dir can be given as command line using "O="
+#
+ifdef O
+ifeq ("$(origin O)", "command line")
+RTE_OUTPUT := $(abspath $(O))
+endif
+endif
+RTE_OUTPUT ?= $(RTE_SRCDIR)/build
+export RTE_OUTPUT
+
+# if we are building an external application, include SDK
+# configuration and include project configuration if any
+include $(RTE_SDK_BIN)/.config
+ifneq ($(wildcard $(RTE_OUTPUT)/.config),)
+  include $(RTE_OUTPUT)/.config
+endif
+# remove double-quotes from config names
+RTE_ARCH := $(CONFIG_RTE_ARCH:"%"=%)
+RTE_MACHINE := $(CONFIG_RTE_MACHINE:"%"=%)
+RTE_EXEC_ENV := $(CONFIG_RTE_EXEC_ENV:"%"=%)
+RTE_TOOLCHAIN := $(CONFIG_RTE_TOOLCHAIN:"%"=%)
+
+
diff --git a/mk/rte.extvars.mk b/mk/rte.extvars.mk
deleted file mode 100644
index 3e5a990..000
--- a/mk/rte.extvars.mk
+++ /dev/null
@@ -1,81 +0,0 @@
-#   BSD LICENSE
-#
-#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
-#   All rights reserved.
-#
-#   Redistribution and use in source and binary forms, with or without
-#   modification, are permitted provided that the following conditions
-#   are met:
-#
-# * Redistributions of source code must retain the above copyright
-#   notice, this list of conditions and the following disclaimer.
-# * Redistributions in binary form must reproduce the above copyright
-#   notice, this list of conditions and the following disclaimer in
-#   the documentation and/or other materials provided with the
-#   distribution.
-# * Neither the name of Intel Corporation nor the names of its
-#   contributors may be used to endorse or promote products derived
-#   from this software without specific prior written permission.
-#
-#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-#   

[dpdk-dev] [PATCH] pci: save list of detached devices, and re-probe during driver unload

[Raz Amir]

Thank you.

See answers inline (mostly ack, but not only), and I will send the updated
patch soon.



> -Original Message-

> From: Bruce Richardson [mailto:bruce.richardson at intel.com]

> Sent: 03 March 2015 15:33

> To: Raz Amir

> Cc: dev at dpdk.org

> Subject: Re: [dpdk-dev] [PATCH] pci: save list of detached devices, and
re-

> probe during driver unload

> 

> On Thu, Feb 26, 2015 at 06:33:20AM +, Raz Amir wrote:

> > Added code that saves the pointers to the detached devices, during

> > driver loading, and during driver unloading, go over the list, and

> > re-attach them by calling device_probe_and_attach on each device.

> >

> > Signed-off-by: Raz Amir < 
razamir22 at gmail.com>

> 

> Couple of minor comments below. Otherwise all looks good to me.

> 

> Acked-by: Bruce Richardson < 
bruce.richardson at intel.com>

> > ---

> >  lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 26

> > +-

> >  1 file changed, 25 insertions(+), 1 deletion(-)

> >

> > diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c

> > b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c

> > index 5ae8560..7d702a5 100644

> > --- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c

> > +++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c

> > @@ -55,6 +55,9 @@ __FBSDID("$FreeBSD$");

> >

> >  #define MAX_BARS (PCIR_MAX_BAR_0 + 1)

> >

> > +#define MAX_DETACHED_DEVICES   128

> > +static device_t detached_devices[MAX_DETACHED_DEVICES] = {}; static

> > +int last_detached = 0;

> Maybe num_detached/nb_detached or even just "detached" instead of

> "last_detached".

Ack.



> 

> >

> >  struct nic_uio_softc {

> >  device_tdev_t;

> > @@ -291,14 +294,35 @@ nic_uio_load(void)

> >  if (dev != NULL)

> 

> We are getting into some serious levels of indentation below, so maybe
flip

> this condition around and put in a "continue" instead, so that we can
dedent

> everything below that follows it.

> 

Ack.



> >  for (i = 0; i < NUM_DEVICES;
i++)

> >  if
(pci_get_vendor(dev) == devices[i].vend

> &&

> > -
pci_get_device(dev) ==

> devices[i].dev)

> > +
pci_get_device(dev) ==

> devices[i].dev) {

> > +
if (last_detached+1 <

> MAX_DETACHED_DEVICES) {

> I don't think you need the +1 here.

It is needed, otherwise the last object will be added at
MAX_DETACHED_DEVICES position while the last position is
MAX_DETACHED_DEVICES-1.

However, I did change the code to:

if (last_detached < MAX_DETACHED_DEVICES-1) {

to better reflect that.

You will see it in the next patch I will send.



> 

> > +

> printf("nic_uio_load: detaching and storing dev=%p\n", dev);

> > +

> detached_devices[last_detached++] = dev;

> > +
}

> > +
else {

> > +

> printf("nic_uio_load: reached MAX_DETACHED_DEVICES=%d.

> dev=%p won't be reattached\n",

> > +

> MAX_DETACHED_DEVICES, dev);

> > +
}

> DPDK coding style is not to put braces around single-line statements like
this.

Ack.



> 

> 

> > +

> Remove whitespace from this new line.

> 

Ack.



> >
device_detach(dev);

> > +  }

> >  }

> >  }

> >

> >  static void

> >  nic_uio_unload(void)

> >  {

> > +  int i;

> > +  printf("nic_uio_unload: entered ... \n");

> > +

> > +  for (i = 0; i < last_detached; i++) {

> > +  printf("nic_uio_unload: calling to
device_probe_and_attach

> for dev=%p...\n",

> > +  detached_devices[i]);

> > +  device_probe_and_attach(detached_devices[i]);

> > +  printf("nic_uio_unload: done.\n");

> > +  }

> > +

> > +  printf("nic_uio_unload: leaving ... \n");

> >  }

> >

> >  static int

> > --

> > 2.1.2

> >

[dpdk-dev] [PATCH 1/7] mempool: fix build with debug enabled

[Olivier MATZ]

Hi Thomas,

On 03/03/2015 04:23 PM, Thomas Monjalon wrote:
> error: format ?%p? expects argument of type ?void *?,
> but argument 5 has type ?const struct rte_mempool *? [-Werror=format=]
>
> mp type is (const struct rte_mempool *) and must be casted into a simpler
> type to be printed.

I was a bit surprised to see this warning although the standard says:

  The argument shall be a pointer to void. The value of the pointer is
  converted to a sequence of printing wide characters, in an
  implementation-defined manner.

But I think we often do this in dpdk, without any warning:

struct foo_s *foo = ...;
printf("%p\n", foo);

After some search, the reason why you get a warning here is that you
compile a C file that includes this header with the "-pedantic" flag.
So, I think doing this fix is justified for header files as we cannot
predict which options are used by the user of these headers.

So:
Acked-by: Olivier Matz 

[dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type in rte_mbuf

[Chilikin, Andrey]

> -Original Message-
> From: Zhang, Helin
> Sent: Wednesday, March 4, 2015 8:34 AM
> To: Chilikin, Andrey; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type
> in rte_mbuf
> 
> 
> 
> > -Original Message-
> > From: Chilikin, Andrey
> > Sent: Monday, March 2, 2015 7:48 PM
> > To: Zhang, Helin; dev at dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of
> > packet_type in rte_mbuf
> >
> > Hi Helin,
> >
> > I see that you have removed "uint16_t reserved" member from rte_mbuf:
> >
> > > + uint16_t data_len;/**< Amount of data in segment buffer. */
> > >   uint16_t vlan_tci;/**< VLAN Tag Control Identifier (CPU order)
> > */
> > > - uint16_t reserved;
> > >   union {
> > >   uint32_t rss; /**< RSS hash result if RSS enabled */
> >
> > This reserved field was kept next to  vlan_tci as a placeholder for
> > the second VLAN label for QinQ support so if need be vlan_tci +
> > reserved could be casted to
> > 32 bit QinQ value or one 32bit VNTAG label. Without keeping two label
> > adjusted to each other casting to 32 bit will not be possible and will
> > affect QinQ performance.
> Yes, but packet type is quite important which needs to be extended from 16
> bits to 32 bits.
> For FVL, the vlan tags are in different fields. 
I do not see how FVL internal descriptor can affect DPDK mbuf structure.

> We can think of putting them
> together in mbuf, Possibly move current vlan tag down, and add one more 16
> bits. Let's see what is the best then.
But if we know that we would need this change for QinQ anyway should we move
vlan_tci +reserved 16bits now in this patch instead of testing performance 
twice -
for this and for future patch when we move vlan_tci+reserved?

Regards,
Andrey

> Thanks for the notes!
> 
> Regards,
> Helin
> 
> >
> > Regards,
> > Andrey
> >
> >
> > > -Original Message-
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Helin Zhang
> > > Sent: Friday, February 27, 2015 1:11 PM
> > > To: dev at dpdk.org
> > > Subject: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of
> > > packet_type in rte_mbuf
> > >
> > > In order to unify the packet type, the field of 'packet_type' in
> > > 'struct rte_mbuf' needs to be extended from 16 to 32 bits.
> > > Accordingly, some fields in 'struct rte_mbuf' are re-organized to
> > > support this change for Vector PMD. As 'struct rte_kni_mbuf' for KNI
> > > should be right mapped to 'struct rte_mbuf', it should be modified
> > > accordingly. In addition, Vector PMD of ixgbe is disabled by
> > > default, as 'struct
> > rte_mbuf' changed.
> > >
> > > Signed-off-by: Helin Zhang 
> > > Signed-off-by: Cunming Liang 
> > > ---
> > >  config/common_linuxapp |  2 +-
> > >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  4 ++--
> > >  lib/librte_mbuf/rte_mbuf.h | 23
> > +++---
> > >  3 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > v2 changes:
> > > * Enlarged the packet_type field from 16 bits to 32 bits.
> > > * Redefined the packet type sub-fields.
> > > * Updated the 'struct rte_kni_mbuf' for KNI according to the mbuf
> changes.
> > >
> > > v3 changes:
> > > * Put the mbuf layout changes into a single patch.
> > > * Disabled vector ixgbe PMD by default, as mbuf layout changed.
> > >
> > > diff --git a/config/common_linuxapp b/config/common_linuxapp index
> > > 97f1c9e..97d7bae 100644
> > > --- a/config/common_linuxapp
> > > +++ b/config/common_linuxapp
> > > @@ -166,7 +166,7 @@ CONFIG_RTE_LIBRTE_IXGBE_DEBUG_TX_FREE=n
> > >  CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n
> > >  CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> > >  CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC=y
> > > -CONFIG_RTE_IXGBE_INC_VECTOR=y
> > > +CONFIG_RTE_IXGBE_INC_VECTOR=n
> > >  CONFIG_RTE_IXGBE_RX_OLFLAGS_ENABLE=y
> > >
> > >  #
> > > diff --git
> > > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > index 1e55c2d..bd1cc09 100644
> > > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > > @@ -117,9 +117,9 @@ struct rte_kni_mbuf {
> > >   uint16_t data_off;  /**< Start address of data in segment buffer. */
> > >   char pad1[4];
> > >   uint64_t ol_flags;  /**< Offload features. */
> > > - char pad2[2];
> > > - uint16_t data_len;  /**< Amount of data in segment buffer. */
> > > + char pad2[4];
> > >   uint32_t pkt_len;   /**< Total pkt len: sum of all segment data_len.
> > > */
> > > + uint16_t data_len;  /**< Amount of data in segment buffer. */
> > >
> > >   /* fields on second cache line */
> > >   char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_SIZE)));
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 17ba791..f5b7a8b 100644
> > > --- 

[dpdk-dev] [PATCH v2] tools/dpdk_nic_bind: Fix can't bind virtio-pci issue

[Ouyang Changchun]

The dpdk_nic_bind script will not allow ports to be bound or unbound if none of 
the
kernel modules supported by DPDK is loaded. This patch relaxes this restriction 
by
checking if a DPDK module is actually requested. The example below illustrates 
this
problem:

In virtio test, on the guest
1. Bind virtio port to igb_uio driver;
2. Remove igb_uio module;
3. Bind virtio port to virtio-pci driver, it fails and reports:
   "Error - no supported modules are loaded"

The script should check the to-be-bound driver flag, if it is dpdk 
driver(igb_uio, vfio etc),
and the corresponding module is not loaded, then exit, otherwise, just report a 
warning,
and continue to bind the non-dpdk driver(like virtio-pci) to dev.

Signed-off-by: Changchun Ouyang 
---

Changes in v2:
  -- Update the description.

 tools/dpdk_nic_bind.py | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/dpdk_nic_bind.py b/tools/dpdk_nic_bind.py
index 2483056..8523f82 100755
--- a/tools/dpdk_nic_bind.py
+++ b/tools/dpdk_nic_bind.py
@@ -175,8 +175,11 @@ def check_modules():

 # check if we have at least one loaded module
 if True not in [mod["Found"] for mod in mods] and b_flag is not None:
-print "Error - no supported modules are loaded"
-sys.exit(1)
+if b_flag in dpdk_drivers:
+print "Error - no supported modules(DPDK driver) are loaded"
+sys.exit(1)
+else:
+print "Warning - no supported modules(DPDK driver) are loaded"

 # change DPDK driver list to only contain drivers that are loaded
 dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
-- 
1.8.4.2

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Stephen Hemminger]

On Wed, 04 Mar 2015 09:57:24 +0200
Vlad Zolotarov  wrote:

> 
> 
> On 03/04/15 02:33, Stephen Hemminger wrote:
> > On Tue,  3 Mar 2015 21:48:43 +0200
> > Vlad Zolotarov  wrote:
> >
> >> +  next_desc:
> >> +  /*
> >> +   * The code in this whole file uses the volatile pointer to
> >> +   * ensure the read ordering of the status and the rest of the
> >> +   * descriptor fields (on the compiler level only!!!). This is so
> >> +   * UGLY - why not to just use the compiler barrier instead? DPDK
> >> +   * even has the rte_compiler_barrier() for that.
> >> +   *
> >> +   * But most importantly this is just wrong because this doesn't
> >> +   * ensure memory ordering in a general case at all. For
> >> +   * instance, DPDK is supposed to work on Power CPUs where
> >> +   * compiler barrier may just not be enough!
> >> +   *
> >> +   * I tried to write only this function properly to have a
> >> +   * starting point (as a part of an LRO/RSC series) but the
> >> +   * compiler cursed at me when I tried to cast away the
> >> +   * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm
> >> +   * keeping it the way it is for now.
> >> +   *
> >> +   * The code in this file is broken in so many other places and
> >> +   * will just not work on a big endian CPU anyway therefore the
> >> +   * lines below will have to be revisited together with the rest
> >> +   * of the ixgbe PMD.
> >> +   *
> >> +   * TODO:
> >> +   *- Get rid of "volatile" crap and let the compiler do its
> >> +   *  job.
> >> +   *- Use the proper memory barrier (rte_rmb()) to ensure the
> >> +   *  memory ordering below.
> > This comment screams "this is broken".
> > Why not get proper architecture independent barriers in DPDK first.
> 
> This series is orthogonal to the issue above. I just couldn't stand to 
> mention this ugliness when I noticed it on the way.
> Note that although this is obviously not the right way to write this 
> kind of code it is still not a bug and most likely the performance 
> implications are minimal here.
> The only overhead is that there may be read "too much" data from the 
> descriptor that we may not actually need. The descriptor is 16 bytes so 
> this doesn't seem to be a critical issue.
> 
> So, fixing the above issue may wait, especially since the same s..t may 
> be found in other Intel PMDs (see i40e for example). Fixing this issue 
> should be a matter of a massive cleanup series that cover all the 
> relevant PMDs. Of course we may start with ixgbe but even in this single 
> PMD there are at least 3 non-LRO related functions that have to be 
> fixed, so IMHO even fixing ONLY ixgbe should be a matter of a separate 
> series.

In userspace-rcu and kernel there is a simple macro that would make this
kind of code more sane.

What about adding:

#define rte_access_once(x)  (*(volatile typeof(x) *)&(x))

Then doing
rxdp = rte_access_once(rx_ring + idx);

[dpdk-dev] Regarding dpdk_qat example

[Prashant Upadhyaya]

Hi,

In the dpdk_qat example, the function alloc_memzone_region does the
allocation for the memory of a crypto session context. Now in a real
application, the sessions will be torn down as well. So if a similar
strategy is followed as that of alloc_memzone_region, then how can the
memory be returned to memzone. I see the comment over the
alloc_memzone_region function which indicates that the allocation is meant
to exist for a lifetime and there is no possibility to free. Or should a
real application follow a different strategy for allocating memory to
sessions, would like to know the advice of QAT users.

Regards
-Prashant

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Stephen Hemminger]

On Wed, 04 Mar 2015 09:59:38 +0200
Vlad Zolotarov  wrote:

> > Checkpatch warnings (edited to remove ones that should be ok)  
> 
> I was unaware that checkpatch rules apply here - at least looking at the 
> current code it looks like it... ;)
> But I'm all for it! I'll fix all the issues and respin

Checkpatch doesn't generally have to apply. But is good and useful tool
to find obvious style gaffs.

Just don't let it rule your world

[dpdk-dev] [PATCH] ring: cleanup file-local macros at end-of-file

[Bruce Richardson]

On Wed, Mar 04, 2015 at 10:26:24AM +, Ananyev, Konstantin wrote:
> 
> 
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Wednesday, March 04, 2015 10:23 AM
> > To: Thomas Monjalon
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ring: cleanup file-local macros at 
> > end-of-file
> > 
> > On Tue, Mar 03, 2015 at 10:03:45PM +0100, Thomas Monjalon wrote:
> > > 2015-03-03 16:38, Bruce Richardson:
> > > > The ENQUEUE_PTRS and DEQUEUE_PTRS macros defined in rte_ring.h are
> > > > not meant to be global and are not prefixed with the RTE_ prefix.
> > > > Therefore undef the macros at end of file to avoid pollution of the
> > > > global namespace, in case ends apps end up wanting to reuse those names.
> > > >
> > > > Signed-off-by: Bruce Richardson 
> > > > ---
> > > >  lib/librte_ring/rte_ring.h | 4 
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > > > index bdf69b7..0d35648 100644
> > > > --- a/lib/librte_ring/rte_ring.h
> > > > +++ b/lib/librte_ring/rte_ring.h
> > > > @@ -1232,6 +1232,10 @@ rte_ring_dequeue_burst(struct rte_ring *r, void 
> > > > **obj_table, unsigned n)
> > > > return rte_ring_mc_dequeue_burst(r, obj_table, n);
> > > >  }
> > > >
> > > > +/* undef un-prefixed macros which are local to this file */
> > > > +#undef ENQUEUE_PTRS
> > > > +#undef DEQUEUE_PTRS
> > > > +
> > >
> > > Thanks for trying to clean-up things.
> > > Note that if an application is using this macro name, it will be destroyed
> > > when including rte_ring.h.
> > > Globally, DPDK namespace is awful and I hope we will be able to improve 
> > > it.
> > >
> > Only if they are defining such a macro before including rte_ring.h, which I 
> > would
> > expect to be an edge case. Also, in such a case, the compiler/preprocessor 
> > will
> > give an error at the duplicate macro definition stage, and the simple fix 
> > is to
> > reorder the header file inclusion to avoid problems i.e. no changing of dpdk
> > required.
> > I suppose a better fix to go along with this is to RTE-prefix the macros. 
> > I'll
> > see about doing a V2.
> 
> Why not just to rename it to __RTE_RING_ENQUEUE_PTRS__ or something,
> and put in the comments that it is for internal usage?
> Konstantin

Yes, I'm planning on renaming it. But since it's for internal use only, I might
as well undef it at the end of the file too. I think it's good practice.

/Bruce

[dpdk-dev] [PATCH] librte_lpm: define tbl entry reversely for big endian

[Bruce Richardson]

On Wed, Mar 04, 2015 at 02:34:12PM +0800, xuelin.shi at freescale.com wrote:
> From: Xuelin Shi 
> 
> This module uses type conversion between struct and int.
> Also truncation and comparison is used with this int.
> It is not safe for different endian arch.
> 
> Add ifdef for big endian struct to fix this issue.
> 
> Signed-off-by: Xuelin Shi 
> ---
>  lib/librte_lpm/rte_lpm.h | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/lib/librte_lpm/rte_lpm.h b/lib/librte_lpm/rte_lpm.h
> index 1af150c..08a2859 100644
> --- a/lib/librte_lpm/rte_lpm.h
> +++ b/lib/librte_lpm/rte_lpm.h
> @@ -96,6 +96,7 @@ extern "C" {
>  /** Bitmask used to indicate successful lookup */
>  #define RTE_LPM_LOOKUP_SUCCESS  0x0100
>  
> +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
>  /** @internal Tbl24 entry structure. */
>  struct rte_lpm_tbl24_entry {
>   /* Stores Next hop or group index (i.e. gindex)into tbl8. */
> @@ -117,6 +118,24 @@ struct rte_lpm_tbl8_entry {
>   uint8_t valid_group :1; /**< Group validation flag. */
>   uint8_t depth   :6; /**< Rule depth. */
>  };
> +#else
> +struct rte_lpm_tbl24_entry {
> + uint8_t depth   :6;
> + uint8_t ext_entry   :1;
> + uint8_t valid   :1;

Since endianness only refers to the order of bytes within a word, do the 
bitfields
within the uint8_t really need to be swapped around too?

/Bruce

> + union {
> + uint8_t tbl8_gindex;
> + uint8_t next_hop;
> + };
> +};
> +
> +struct rte_lpm_tbl8_entry {
> + uint8_t depth   :6;
> + uint8_t valid_group :1;
> + uint8_t valid   :1;
> + uint8_t next_hop;
> +};
> +#endif
>  
>  /** @internal Rule structure. */
>  struct rte_lpm_rule {
> -- 
> 1.9.1
> 

[dpdk-dev] [PATCH] mk: add support for gdb debug info generation

[Olivier MATZ]

Hi Marc,

On 03/03/2015 02:27 PM, Marc Sune wrote:
>
> On 03/03/15 14:03, Bruce Richardson wrote:
>> On Tue, Mar 03, 2015 at 01:56:19PM +0100, Marc Sune wrote:
>> [...]
>> I believe that the global option of overriding the CFLAGS is already
>> sufficiently
>> covered - including being documented in programmers guide - by
>> EXTRA_CFLAGS.
>
> To be honest, I tried EXTRA_CFLAGS at some point in time (probably 1.5
> or 1.6, but maybe not stable releases) and it did not work, so I ended
> up doing it manually, and never tried again.
>
> It does work now with CFLAGS, I didn't try LDFLAGS, but it does not for
> EXTRA_CPPFLAGS apparently (unless I made some stupid mistake):
>
> marc at dpdk:~/dpdk$ git diff
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index 4e70fa0..4a1e538 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -61,6 +61,10 @@
>
>   #define KNI_MEM_CHECK(cond) do { if (cond) goto kni_fail; } while (0)
>
> +#ifdef TEST_CPPFLAGS
> +   #error TEST_CPPFLAGS defined
> +#endif
> +
>   /**
>* KNI context
>*/
>
> marc at dpdk:~/dpdk$ export EXTRA_CPPFLAGS='-DTEST_CPPFLAGS'
> marc at dpdk:~/dpdk$ make install T=x86_64-native-linuxapp-gcc
> ...
> Build complete

The reason why it does not work is described in the documentation:

./doc/guides/prog_guide/build_app.rst:*   CPPFLAGS: The flags to use to 
provide flags to the C preprocessor (only useful when assembling .S files)
./doc/guides/prog_guide/dev_kit_build_system.rst:*   CPPFLAGS: Flags to 
use to give flags to C preprocessor (only useful when assembling .S files).
./doc/guides/prog_guide/dev_kit_build_system.rst:*   EXTRA_CPPFLAGS: The 
content of this variable is appended after CPPFLAGS when using a C 
preprocessor on assembly files.

I think your test would work with EXTRA_CFLAGS.

I don't say it's the proper behavior, but at least it's coherent with
the documentation.

Regards,
Olivier

[dpdk-dev] [PATCH] config: default to shared library

[Bruce Richardson]

On Wed, Mar 04, 2015 at 10:24:44AM +0100, Thomas Monjalon wrote:
> Hi Panu,
> 
> 2015-03-04 08:17, Panu Matilainen:
> > With symbol versioning its vital that developers test their code in
> > shared library mode, otherwise we'll be playing "add the forgotten
> > symbol export" from here to eternity.
> 
> Yes we must improve the sanity checks.
> A lot of options must be tested (or removed) and not only shared libs.
> But the error you reported before (missing export of rte_eth_dev_release_port)
> cannot be seen even with this patch.
> It means we need more tools.
> Though, default configuration is not a tool.
> 
> > By defaulting to shared we should catch more of these cases early,
> > but without taking away anybodys ability to build static.
> 
> Shared libraries are convenient for distributions but have a performance
> impact. I think that static build must remain the default choice.
> 
+1

[dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Neil Horman]

On Wed, Mar 04, 2015 at 04:15:18PM +0100, Thomas Monjalon wrote:
> 2015-03-04 09:39, Neil Horman:
> > On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> > > Hi Neil,
> > > 
> > > I remove parts that I agree and reply to those which deserve more 
> > > discussion.
> > > 
> > > 2015-03-04 06:49, Neil Horman:
> > > > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > > > 2015-02-02 13:18, Neil Horman:
> > > > > > +# Validate that we have all the arguments we need
> > > > > > +if [ ! -d ./.git ]
> > > > > > +then
> > > > > > +   log "WARN" "You must be in the root of the dpdk git tree"
> > > > > > +   log "WARN" "You are in $PWD"
> > > > > > +   cleanup_and_exit 1
> > > > > > +fi
> > > > > 
> > > > > Why not cd $(dirname $0)/.. instead of returning an error?
> > > > 
> > > > Why would that help in finding the base of the git tree.  Theres no 
> > > > guarantee
> > > > that you are in a subdirectory of a git tree.  I suppose we can try it
> > > > recursively until we hit /, but it seems just as easy and clear to tell 
> > > > the user
> > > > whats needed.
> > > 
> > > No I'm saying that you could avoid this check by going into the right
> > > directory from the beginning.
> > > We know that the root dir is $(dirname $0)/.. because this script is in
> > > scripts/ directory.
> > > 
> > That only helps if you start from the right directory.  If you run this 
> > command
> > from some other location, your solution just breaks.
> 
> Why it would break? $(dirname $0) is always reachable because you launched $0.
> The only exception is for the case the PATH variable is used to find the DPDK
> scripts/ directory (should not happen).
> 
Ah!  Sorry, misunderstood, for some reason I was conflating $0 with $PWD.  Yes,
this will work and I'll update it

> > > > > > +# Make sure we configure SHARED libraries
> > > > > > +# Also turn off IGB and KNI as those require kernel headers to 
> > > > > > build
> > > > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" 
> > > > > > config/defconfig_$TARGET
> > > > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > > > 
> > > > > Why not tuning configuration after make config in .config file?
> > > > > 
> > > > Because this way we save a reconfig (from a developer viewpoint), you 
> > > > should run
> > > > make config again after changing configs, and so this way you save 
> > > > doing that.
> > > 
> > > No, you run make config once and update .config file. That's the 
> > > recommended
> > > way to configure DPDK.
> > > defconfig files are default configurations and should stay read-only.
> > 
> > They get overwritten when we do the git resets.  Its silly to modify your 
> > config
> > file after you run make config, in the event the make target has to re-read 
> > any
> > modified options and adjust dependent config files accordingly.  I 
> > understand
> > that doesn't happen now, but its common practice for every open source 
> > project
> > in existance.
> 
> I'm not sure to understand. Maybe an example would help.
> By the way, your method works.
For example, the linux kernel.  The .config file that is generated in the root
directory is converted to an autoconf.h in parallel with its generation, for
applications to key off of.  If you change something in .config, you need to run
make config again so that those changes are reflected into the other
auto-generated files.  Thats common practice.  So its counter intuitive to
assume that altering the generated .config file is automatically recognized by
the rest of the build, without a subsequent make config (be it explicit or and
implicit dependency of the make all target).

Neil


> 
> 

[dpdk-dev] Build failure on FreeBSD-10.1-RELEASE

[Olivier MATZ]

Hi Tetsuya, Hi Bruce,

On 03/04/2015 04:34 AM, Tetsuya Mukawa wrote:
> On 2015/03/02 19:22, Bruce Richardson wrote:
>> On Mon, Mar 02, 2015 at 12:47:42PM +0900, Tetsuya Mukawa wrote:
>>> Hi,
>>>
>>> I got a error while building master branch on FreeBSD.
>>> Here is a log.
>>>
>>> $ gmake T=x86_64-native-bsdapp-clang config
>>> cc: error: unknown argument: '-fdirectives-only'
>>> cp: /usr/home/mukawa/work/dpdk/build/.config_tmp: No such file or directory
>>> cp: /usr/home/mukawa/work/dpdk/build/.config_tmp: No such file or directory
>>> gmake[3]: Nothing to be done for 'depdirs'.
>>> Configuration done
>>>
>>>
>>> Here is log came from 'uname'
>>>
>>> $ uname -a
>>> FreeBSD eris.hq.igel.co.jp 10.1-RELEASE FreeBSD 10.1-RELEASE #0 r274401:
>>> Tue Nov 11 21:02:49 UTC 2014
>>>
>>>
>>> I've tried to remove '-fdirectives-only' from mk/rte.sdkconfig.mk like
>>> below.
>>> With the fixing,  It seems I can compile and run testpmd.
>>> (Obviously, we should not merge below patch, but I've done just for testing)
>>>
>>> diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk
>>> index d43c430..f8d95b1 100644
>>> --- a/mk/rte.sdkconfig.mk
>>> +++ b/mk/rte.sdkconfig.mk
>>> @@ -75,7 +75,7 @@ else
>>>   $(RTE_OUTPUT)/.config: $(RTE_CONFIG_TEMPLATE) FORCE | $(RTE_OUTPUT)
>>>  $(Q)if [ "$(RTE_CONFIG_TEMPLATE)" != "" -a -f
>>> "$(RTE_CONFIG_TEMPLATE)" ]; then \
>>>  $(CPP) -undef -P -x assembler-with-cpp \
>>> -   -fdirectives-only -ffreestanding \
>>> +   -ffreestanding \
>>>  -o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
>>>  if ! cmp -s $(RTE_OUTPUT)/.config_tmp
>>> $(RTE_OUTPUT)/.config; then \
>>>  cp $(RTE_OUTPUT)/.config_tmp
>>> $(RTE_OUTPUT)/.config ; \
>>>
>>>
>>> Also, I've checked /usr/ports/net/dpdk, and found below line.
>>> (It seems above ports dpdk package is based on DPDK-1.8.)
>>>
>>>
>>>  $(CPP) -undef -P -x assembler-with-cpp \
>>>   -ffreestanding \
>>> -o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
>>>
>>> So, I guess we should not add '-fdirectives-only' for flags of $(CPP)
>>> for BSD system like dpdk package of ports.
>>>
>>> Thanks,
>>> Tetsuya
>>>
>> Yes, that is correct. In most cases I have tested, the extra flag only gives 
>> a
>> warning but it appears its now an error. We should conditionally include or
>> omit the flag for BSD vs Linux, I think.
>>
>> /Bruce
> Hi Bruce,
>
> It seems we cannot use CONFIG_RTE_EXEC_ENV_LINUXAPP/BSDAPP definition here.
> Now I am looking for other way to check target OS.
> Is it not so good to use $(T) definition value here?

Indeed, it seems that the -fdirectives-only option does not exist in
freebsd. This is probably because the default cpp is not GNU cpp:

On my version, I have:
  FreeBSD clang version 3.3 (tags/RELEASE_33/final 183502) 20130610
  Target: x86_64-unknown-freebsd10.0
  Thread model: posix

To decide whether using the option, we could check the return value of
cpp -fdirectives-only /dev/null > /dev/null 2>/dev/null

But I don't really think it's an issue to remove the option for all
OSes. In my opinion, expanding macros when parsing the config files
won't add any issue, and it's probably better to have no differences
between FreeBSD and Linux.

Regards,
Olivier

[dpdk-dev] [PATCH] ring: cleanup file-local macros at end-of-file

[Ananyev, Konstantin]

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Bruce Richardson
> Sent: Wednesday, March 04, 2015 10:23 AM
> To: Thomas Monjalon
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ring: cleanup file-local macros at end-of-file
> 
> On Tue, Mar 03, 2015 at 10:03:45PM +0100, Thomas Monjalon wrote:
> > 2015-03-03 16:38, Bruce Richardson:
> > > The ENQUEUE_PTRS and DEQUEUE_PTRS macros defined in rte_ring.h are
> > > not meant to be global and are not prefixed with the RTE_ prefix.
> > > Therefore undef the macros at end of file to avoid pollution of the
> > > global namespace, in case ends apps end up wanting to reuse those names.
> > >
> > > Signed-off-by: Bruce Richardson 
> > > ---
> > >  lib/librte_ring/rte_ring.h | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > > index bdf69b7..0d35648 100644
> > > --- a/lib/librte_ring/rte_ring.h
> > > +++ b/lib/librte_ring/rte_ring.h
> > > @@ -1232,6 +1232,10 @@ rte_ring_dequeue_burst(struct rte_ring *r, void 
> > > **obj_table, unsigned n)
> > >   return rte_ring_mc_dequeue_burst(r, obj_table, n);
> > >  }
> > >
> > > +/* undef un-prefixed macros which are local to this file */
> > > +#undef ENQUEUE_PTRS
> > > +#undef DEQUEUE_PTRS
> > > +
> >
> > Thanks for trying to clean-up things.
> > Note that if an application is using this macro name, it will be destroyed
> > when including rte_ring.h.
> > Globally, DPDK namespace is awful and I hope we will be able to improve it.
> >
> Only if they are defining such a macro before including rte_ring.h, which I 
> would
> expect to be an edge case. Also, in such a case, the compiler/preprocessor 
> will
> give an error at the duplicate macro definition stage, and the simple fix is 
> to
> reorder the header file inclusion to avoid problems i.e. no changing of dpdk
> required.
> I suppose a better fix to go along with this is to RTE-prefix the macros. I'll
> see about doing a V2.

Why not just to rename it to __RTE_RING_ENQUEUE_PTRS__ or something,
and put in the comments that it is for internal usage?
Konstantin
> 
> /Bruce

[dpdk-dev] [PATCH] config: default to shared library

[Thomas Monjalon]

Hi Panu,

2015-03-04 08:17, Panu Matilainen:
> With symbol versioning its vital that developers test their code in
> shared library mode, otherwise we'll be playing "add the forgotten
> symbol export" from here to eternity.

Yes we must improve the sanity checks.
A lot of options must be tested (or removed) and not only shared libs.
But the error you reported before (missing export of rte_eth_dev_release_port)
cannot be seen even with this patch.
It means we need more tools.
Though, default configuration is not a tool.

> By defaulting to shared we should catch more of these cases early,
> but without taking away anybodys ability to build static.

Shared libraries are convenient for distributions but have a performance
impact. I think that static build must remain the default choice.

[dpdk-dev] [PATCH] ring: cleanup file-local macros at end-of-file

[Bruce Richardson]

On Tue, Mar 03, 2015 at 10:03:45PM +0100, Thomas Monjalon wrote:
> 2015-03-03 16:38, Bruce Richardson:
> > The ENQUEUE_PTRS and DEQUEUE_PTRS macros defined in rte_ring.h are
> > not meant to be global and are not prefixed with the RTE_ prefix.
> > Therefore undef the macros at end of file to avoid pollution of the
> > global namespace, in case ends apps end up wanting to reuse those names.
> > 
> > Signed-off-by: Bruce Richardson 
> > ---
> >  lib/librte_ring/rte_ring.h | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/lib/librte_ring/rte_ring.h b/lib/librte_ring/rte_ring.h
> > index bdf69b7..0d35648 100644
> > --- a/lib/librte_ring/rte_ring.h
> > +++ b/lib/librte_ring/rte_ring.h
> > @@ -1232,6 +1232,10 @@ rte_ring_dequeue_burst(struct rte_ring *r, void 
> > **obj_table, unsigned n)
> > return rte_ring_mc_dequeue_burst(r, obj_table, n);
> >  }
> >  
> > +/* undef un-prefixed macros which are local to this file */
> > +#undef ENQUEUE_PTRS
> > +#undef DEQUEUE_PTRS
> > +
> 
> Thanks for trying to clean-up things.
> Note that if an application is using this macro name, it will be destroyed
> when including rte_ring.h.
> Globally, DPDK namespace is awful and I hope we will be able to improve it.
> 
Only if they are defining such a macro before including rte_ring.h, which I 
would
expect to be an edge case. Also, in such a case, the compiler/preprocessor will
give an error at the duplicate macro definition stage, and the simple fix is to
reorder the header file inclusion to avoid problems i.e. no changing of dpdk 
required.
I suppose a better fix to go along with this is to RTE-prefix the macros. I'll 
see about doing a V2.

/Bruce

[dpdk-dev] [PATCH] A fix to work around strict-aliasing rules breaking

[Bruce Richardson]

On Wed, Mar 04, 2015 at 02:07:20AM +, Wang, Zhihong wrote:
> 
> 
> > -Original Message-
> > From: Richardson, Bruce
> > Sent: Monday, March 02, 2015 6:32 PM
> > To: Wang, Zhihong
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] A fix to work around strict-aliasing rules
> > breaking
> > 
> > On Mon, Mar 02, 2015 at 05:03:50PM +0800, zhihong.wang at intel.com wrote:
> > > Fixed strict-aliasing rules breaking errors for some GCC version.
> > >
> > 
> > This looks messy. Also, I believe the definition of memcpy should include 
> > the
> > "restrict" keyword to indicate that source and dest can't overlap. Might 
> > that
> > help fix the issue?
> 
> It's actually caused by casting void * to multiple other pointer types.
> 
Yes, because two pointers of different types are not allowed to point to the
same memory. If the two pointers of different types are belonging to the two
different variables, the "restrict" keyword may indeed help, but that's probably
not the case here.

/Bruce
> > 
> > /Bruce
> > 
> > > Signed-off-by: Zhihong Wang 
> > > ---
> > >  .../common/include/arch/x86/rte_memcpy.h   | 44 
> > --
> > >  1 file changed, 24 insertions(+), 20 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 69a5c6f..f412099 100644
> > > --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > > +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > > @@ -195,6 +195,8 @@ rte_mov256blocks(uint8_t *dst, const uint8_t *src,
> > > size_t n)  static inline void *  rte_memcpy(void *dst, const void
> > > *src, size_t n)  {
> > > + uintptr_t dstu = (uintptr_t)dst;
> > > + uintptr_t srcu = (uintptr_t)src;
> > >   void *ret = dst;
> > >   int dstofss;
> > >   int bits;
> > > @@ -204,22 +206,22 @@ rte_memcpy(void *dst, const void *src, size_t n)
> > >*/
> > >   if (n < 16) {
> > >   if (n & 0x01) {
> > > - *(uint8_t *)dst = *(const uint8_t *)src;
> > > - src = (const uint8_t *)src + 1;
> > > - dst = (uint8_t *)dst + 1;
> > > + *(uint8_t *)dstu = *(const uint8_t *)srcu;
> > > + srcu = (uintptr_t)((const uint8_t *)srcu + 1);
> > > + dstu = (uintptr_t)((uint8_t *)dstu + 1);
> > >   }
> > >   if (n & 0x02) {
> > > - *(uint16_t *)dst = *(const uint16_t *)src;
> > > - src = (const uint16_t *)src + 1;
> > > - dst = (uint16_t *)dst + 1;
> > > + *(uint16_t *)dstu = *(const uint16_t *)srcu;
> > > + srcu = (uintptr_t)((const uint16_t *)srcu + 1);
> > > + dstu = (uintptr_t)((uint16_t *)dstu + 1);
> > >   }
> > >   if (n & 0x04) {
> > > - *(uint32_t *)dst = *(const uint32_t *)src;
> > > - src = (const uint32_t *)src + 1;
> > > - dst = (uint32_t *)dst + 1;
> > > + *(uint32_t *)dstu = *(const uint32_t *)srcu;
> > > + srcu = (uintptr_t)((const uint32_t *)srcu + 1);
> > > + dstu = (uintptr_t)((uint32_t *)dstu + 1);
> > >   }
> > >   if (n & 0x08) {
> > > - *(uint64_t *)dst = *(const uint64_t *)src;
> > > + *(uint64_t *)dstu = *(const uint64_t *)srcu;
> > >   }
> > >   return ret;
> > >   }
> > > @@ -458,6 +460,8 @@ static inline void *  rte_memcpy(void *dst, const
> > > void *src, size_t n)  {
> > >   __m128i xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7,
> > xmm8;
> > > + uintptr_t dstu = (uintptr_t)dst;
> > > + uintptr_t srcu = (uintptr_t)src;
> > >   void *ret = dst;
> > >   int dstofss;
> > >   int srcofs;
> > > @@ -467,22 +471,22 @@ rte_memcpy(void *dst, const void *src, size_t n)
> > >*/
> > >   if (n < 16) {
> > >   if (n & 0x01) {
> > > - *(uint8_t *)dst = *(const uint8_t *)src;
> > > - src = (const uint8_t *)src + 1;
> > > - dst = (uint8_t *)dst + 1;
> > > + *(uint8_t *)dstu = *(const uint8_t *)srcu;
> > > + srcu = (uintptr_t)((const uint8_t *)srcu + 1);
> > > + dstu = (uintptr_t)((uint8_t *)dstu + 1);
> > >   }
> > >   if (n & 0x02) {
> > > - *(uint16_t *)dst = *(const uint16_t *)src;
> > > - src = (const uint16_t *)src + 1;
> > > - dst = (uint16_t *)dst + 1;
> > > + *(uint16_t *)dstu = *(const uint16_t *)srcu;
> > > + srcu = (uintptr_t)((const uint16_t *)srcu + 1);
> > > + dstu = (uintptr_t)((uint16_t *)dstu + 1);
> > >   }
> > >   if (n & 0x04) {
> > > - *(uint32_t *)dst = *(const uint32_t *)src;
> > > - src = (const uint32_t *)src + 1;
> > > - dst = (uint32_t *)dst + 1;
> > > + 

[dpdk-dev] Build failure on FreeBSD-10.1-RELEASE

[Bruce Richardson]

On Wed, Mar 04, 2015 at 10:33:14AM +0100, Olivier MATZ wrote:
> Hi Tetsuya, Hi Bruce,
> 
> On 03/04/2015 04:34 AM, Tetsuya Mukawa wrote:
> >On 2015/03/02 19:22, Bruce Richardson wrote:
> >>On Mon, Mar 02, 2015 at 12:47:42PM +0900, Tetsuya Mukawa wrote:
> >>>Hi,
> >>>
> >>>I got a error while building master branch on FreeBSD.
> >>>Here is a log.
> >>>
> >>>$ gmake T=x86_64-native-bsdapp-clang config
> >>>cc: error: unknown argument: '-fdirectives-only'
> >>>cp: /usr/home/mukawa/work/dpdk/build/.config_tmp: No such file or directory
> >>>cp: /usr/home/mukawa/work/dpdk/build/.config_tmp: No such file or directory
> >>>gmake[3]: Nothing to be done for 'depdirs'.
> >>>Configuration done
> >>>
> >>>
> >>>Here is log came from 'uname'
> >>>
> >>>$ uname -a
> >>>FreeBSD eris.hq.igel.co.jp 10.1-RELEASE FreeBSD 10.1-RELEASE #0 r274401:
> >>>Tue Nov 11 21:02:49 UTC 2014
> >>>
> >>>
> >>>I've tried to remove '-fdirectives-only' from mk/rte.sdkconfig.mk like
> >>>below.
> >>>With the fixing,  It seems I can compile and run testpmd.
> >>>(Obviously, we should not merge below patch, but I've done just for 
> >>>testing)
> >>>
> >>>diff --git a/mk/rte.sdkconfig.mk b/mk/rte.sdkconfig.mk
> >>>index d43c430..f8d95b1 100644
> >>>--- a/mk/rte.sdkconfig.mk
> >>>+++ b/mk/rte.sdkconfig.mk
> >>>@@ -75,7 +75,7 @@ else
> >>>  $(RTE_OUTPUT)/.config: $(RTE_CONFIG_TEMPLATE) FORCE | $(RTE_OUTPUT)
> >>> $(Q)if [ "$(RTE_CONFIG_TEMPLATE)" != "" -a -f
> >>>"$(RTE_CONFIG_TEMPLATE)" ]; then \
> >>> $(CPP) -undef -P -x assembler-with-cpp \
> >>>-   -fdirectives-only -ffreestanding \
> >>>+   -ffreestanding \
> >>> -o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
> >>> if ! cmp -s $(RTE_OUTPUT)/.config_tmp
> >>>$(RTE_OUTPUT)/.config; then \
> >>> cp $(RTE_OUTPUT)/.config_tmp
> >>>$(RTE_OUTPUT)/.config ; \
> >>>
> >>>
> >>>Also, I've checked /usr/ports/net/dpdk, and found below line.
> >>>(It seems above ports dpdk package is based on DPDK-1.8.)
> >>>
> >>>
> >>> $(CPP) -undef -P -x assembler-with-cpp \
> >>>  -ffreestanding \
> >>>-o $(RTE_OUTPUT)/.config_tmp $(RTE_CONFIG_TEMPLATE) ; \
> >>>
> >>>So, I guess we should not add '-fdirectives-only' for flags of $(CPP)
> >>>for BSD system like dpdk package of ports.
> >>>
> >>>Thanks,
> >>>Tetsuya
> >>>
> >>Yes, that is correct. In most cases I have tested, the extra flag only 
> >>gives a
> >>warning but it appears its now an error. We should conditionally include or
> >>omit the flag for BSD vs Linux, I think.
> >>
> >>/Bruce
> >Hi Bruce,
> >
> >It seems we cannot use CONFIG_RTE_EXEC_ENV_LINUXAPP/BSDAPP definition here.
> >Now I am looking for other way to check target OS.
> >Is it not so good to use $(T) definition value here?
> 
> Indeed, it seems that the -fdirectives-only option does not exist in
> freebsd. This is probably because the default cpp is not GNU cpp:
> 
> On my version, I have:
>  FreeBSD clang version 3.3 (tags/RELEASE_33/final 183502) 20130610
>  Target: x86_64-unknown-freebsd10.0
>  Thread model: posix
> 
> To decide whether using the option, we could check the return value of
> cpp -fdirectives-only /dev/null > /dev/null 2>/dev/null
> 
> But I don't really think it's an issue to remove the option for all
> OSes. In my opinion, expanding macros when parsing the config files
> won't add any issue, and it's probably better to have no differences
> between FreeBSD and Linux.
> 
> Regards,
> Olivier
> 
+1 
I was just going to suggest that this morning! :-)

[dpdk-dev] [PATCH] pci: save list of detached devices, and re-probe during driver unload

[Bruce Richardson]

On Wed, Mar 04, 2015 at 11:07:41AM +0200, Raz Amir wrote:
> Thank you.
> 
> See answers inline (mostly ack, but not only), and I will send the updated
> patch soon.
> 
>  
> 
> > -Original Message-
> 
> > From: Bruce Richardson [mailto:bruce.richardson at intel.com]
> 
> > Sent: 03 March 2015 15:33
> 
> > To: Raz Amir
> 
> > Cc: dev at dpdk.org
> 
> > Subject: Re: [dpdk-dev] [PATCH] pci: save list of detached devices, and
> re-
> 
> > probe during driver unload
> 
> > 
> 
> > On Thu, Feb 26, 2015 at 06:33:20AM +, Raz Amir wrote:
> 
> > > Added code that saves the pointers to the detached devices, during
> 
> > > driver loading, and during driver unloading, go over the list, and
> 
> > > re-attach them by calling device_probe_and_attach on each device.
> 
> > >
> 
> > > Signed-off-by: Raz Amir < 
> razamir22 at gmail.com>
> 
> > 
> 
> > Couple of minor comments below. Otherwise all looks good to me.
> 
> > 
> 
> > Acked-by: Bruce Richardson < 
> bruce.richardson at intel.com>
> 
> > > ---
> 
> > >  lib/librte_eal/bsdapp/nic_uio/nic_uio.c | 26
> 
> > > +-
> 
> > >  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> > >
> 
> > > diff --git a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > index 5ae8560..7d702a5 100644
> 
> > > --- a/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > +++ b/lib/librte_eal/bsdapp/nic_uio/nic_uio.c
> 
> > > @@ -55,6 +55,9 @@ __FBSDID("$FreeBSD$");
> 
> > >
> 
> > >  #define MAX_BARS (PCIR_MAX_BAR_0 + 1)
> 
> > >
> 
> > > +#define MAX_DETACHED_DEVICES   128
> 
> > > +static device_t detached_devices[MAX_DETACHED_DEVICES] = {}; static
> 
> > > +int last_detached = 0;
> 
> > Maybe num_detached/nb_detached or even just "detached" instead of
> 
> > "last_detached".
> 
> Ack.
> 
>  
> 
> > 
> 
> > >
> 
> > >  struct nic_uio_softc {
> 
> > >  device_tdev_t;
> 
> > > @@ -291,14 +294,35 @@ nic_uio_load(void)
> 
> > >  if (dev != NULL)
> 
> > 
> 
> > We are getting into some serious levels of indentation below, so maybe
> flip
> 
> > this condition around and put in a "continue" instead, so that we can
> dedent
> 
> > everything below that follows it.
> 
> > 
> 
> Ack.
> 
>  
> 
> > >  for (i = 0; i < NUM_DEVICES;
> i++)
> 
> > >  if
> (pci_get_vendor(dev) == devices[i].vend
> 
> > &&
> 
> > > -
> pci_get_device(dev) ==
> 
> > devices[i].dev)
> 
> > > +
> pci_get_device(dev) ==
> 
> > devices[i].dev) {
> 
> > > +
> if (last_detached+1 <
> 
> > MAX_DETACHED_DEVICES) {
> 
> > I don't think you need the +1 here.
> 
> It is needed, otherwise the last object will be added at
> MAX_DETACHED_DEVICES position while the last position is
> MAX_DETACHED_DEVICES-1.

Yes, the last position is MAX_DETACHED_DEVICES-1, but you do the addition
of the element to the array using "detached_devices[last_detached++]", i.e. a
post-increment, so when last_detached == (MAX_DETACHED_DEVICES-1), you still
can fill in an entry. Next time around, when last_detached == 
MAX_DETACHED_DEVICES
it's no longer safe to add, and the condition "last_detached < 
MAX_DETACHED_DEVICES)
will now fail. No +1 or -1 necessary to prevent this.

/Bruce

[dpdk-dev] [PATCH] External app builds need to locate common make fragments and includes.

[Olivier MATZ]

Hi Keith,

On 02/28/2015 05:56 PM, Keith Wiles wrote:
> When building an external application like Pktgen and using the proper
> makefile fragments rte.extXYZ.mk NOT rte.XYZ.mk files as you would
> use with example applications in the same RTE_SDK directory the rte.extXYZ.mk
> files are missing some defines/includes.
>
>1 - Add missing tests for RTE_SDK/RTE_TARGET not defined code.
>2 - The build of external applications are forced to be verbose ouput
>as the Q=@ define is not present.
>3 - Missing include of target/generic/rte.vars.mk file which includes
>the information to locate the rte_config.h and other DPDK include
>files.
>
> A patch like this one was submitted before and was rejected because it
> seemed it was not required, because target/generic/rte.vars.mk already
> included by rte.vars.mk.
>
> This is not the cause for external applications like Pktgen which are
> built outside of the RTE_SDK directory and only include the rte.extXYZ.mk
> makefile fragments.

I still not understand what is your problem. If you take an example from
dpdk, let's say examples/l2fwd.

   cd test
   # compile dpdk
   git clone http://dpdk.org/git/dpdk
   cd dpdk
   DPDK=${PWD}
   make -j8 install T=x86_64-native-linuxapp-gcc
   cd ..
   # copy l2fwd in an external directory
   cp -r dpdk/examples/l2fwd .
   cd l2fwd
   # build it
   make RTE_TARGET=x86_64-native-linuxapp-gcc RTE_SDK=${DPDK}

So if you use a Makefile similar to l2fwd, I think it should work.
As I explained in [1], you need to include "rte.vars.mk" at the
beginning of the Makefile, not "rte.extvars.mk". But you will use
"rte.extapp.mk" at the end, like in l2fwd Makefile.

Regards,
Olivier


[1] http://dpdk.org/ml/archives/dev/2015-February/013301.html

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Avi Kivity]

On 03/04/2015 02:33 AM, Stephen Hemminger wrote:
> On Tue,  3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov  wrote:
>
>> + * TODO:
>> + *- Get rid of "volatile" crap and let the compiler do its
>> + *  job.
>> + *- Use the proper memory barrier (rte_rmb()) to ensure the
>> + *  memory ordering below.
> This comment screams "this is broken".
> Why not get proper architecture independent barriers in DPDK first.

C11 has arch independent memory barriers, so this can be as simple as 
-std=gnu11 (default in gcc 5, anyway).

Not only do we get the barriers for free, but they are also properly 
integrated with the compiler, so for example a release barrier won't 
stop the compiler from hoisting a later accesses to before the store, or 
cause spurious reloads, due to the memory clobber.

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Vlad Zolotarov]

On 03/04/15 02:36, Stephen Hemminger wrote:
> On Tue,  3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov  wrote:
>
>>  - Only x540 and 82599 devices support LRO.
>>  - Add the appropriate HW configuration.
>>  - Add RSC aware rx_pkt_burst() handlers:
>> - Implemented bulk allocation and non-bulk allocation versions.
>> - Add LRO-specific fields to rte_eth_rxmode, to rte_eth_dev_data
>>   and to igb_rx_queue.
>> - Use the appropriate handler when LRO is requested.
>>
>> Signed-off-by: Vlad Zolotarov 
> Checkpatch warnings (edited to remove ones that should be ok)

I was unaware that checkpatch rules apply here - at least looking at the 
current code it looks like it... ;)
But I'm all for it! I'll fix all the issues and respin.

Thanks.

>
>
> WARNING: 'recieved' may be misspelled - perhaps 'received'?
> #196: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1426:
> + * @rx_pkts table of recieved packets
>
> WARNING: Missing a blank line after declarations
> #223: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1453:
> + struct igb_rx_queue *rxq = rx_queue;
> + volatile union ixgbe_adv_rx_desc *rx_ring = rxq->rx_ring;
>
>
>
> WARNING: labels should not be indented
> #246: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1476:
> + next_desc:
>
> WARNING: quoted string split across lines
> #285: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1515:
> + PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_id=%u "
> +   "staterr=0x%x data_len=%u",
>
> WARNING: quoted string split across lines
> #293: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1523:
> + PMD_RX_LOG(DEBUG, "RX mbuf alloc failed "
> +   "port_id=%u queue_id=%u",
>
> WARNING: Missing a blank line after declarations
> #302: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1532:
> + uint16_t next_rdt = rxq->rx_free_trigger;
> + if (!ixgbe_rx_alloc_bufs(rxq, false)) {
>
> WARNING: quoted string split across lines
> #309: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1539:
> + PMD_RX_LOG(DEBUG, "RX bulk alloc failed "
> +   "port_id=%u queue_id=%u",
>
> ERROR: code indent should use tabs where possible
> #350: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1580:
> +rxm->data_off = RTE_PKTMBUF_HEADROOM;$
>
> WARNING: please, no spaces at the start of a line
> #350: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1580:
> +rxm->data_off = RTE_PKTMBUF_HEADROOM;$
>
> WARNING: quoted string split across lines
> #452: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:1682:
> + PMD_RX_LOG(DEBUG, "port_id=%u queue_id=%u rx_tail=%u "
> +"nb_hold=%u nb_rx=%u",
>
> WARNING: quoted string split across lines
> #536: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:2580:
> + PMD_INIT_LOG(DEBUG, "sw_ring=%p sw_rsc_ring=%p hw_ring=%p "
> + "dma_addr=0x%"PRIx64,
>
> WARNING: Possible switch case/default not preceeded by break or fallthrough 
> comment
> #617: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3926:
> + default:
>
> WARNING: quoted string split across lines
> #648: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3966:
> + PMD_INIT_LOG(CRIT, "LRO is requested on HW that doesn't "
> +"support it");
>
> WARNING: quoted string split across lines
> #693: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4022:
> + PMD_INIT_LOG(CRIT, "LRO can't be enabled when HW CRC "
> + "is disabled");
>
> WARNING: quoted string split across lines
> #711: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4084:
> + PMD_INIT_LOG(INFO, "split_hdr_size less than "
> +"128 bytes (%d)!",
>
> WARNING: quoted string split across lines
> #835: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4267:
> + PMD_INIT_LOG(INFO, "LRO is requested. Using a bulk "
> +"allocation version");
>
> WARNING: quoted string split across lines
> #840: FILE: lib/librte_pmd_ixgbe/ixgbe_rxtx.c:4272:
> + PMD_INIT_LOG(INFO, "LRO is requested. Using a single "
> +"allocation version");
>
>

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Vlad Zolotarov]

On 03/04/15 02:34, Stephen Hemminger wrote:
> On Tue,  3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov  wrote:
>
>> +
>> +if (!bulk_alloc) {
>> +__le64 dma =
>> +  rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(nmb));
>> +/*
>> + * Update RX descriptor with the physical address of the
>> + * new data buffer of the new allocated mbuf.
>> + */
>> +rxe->mbuf = nmb;
>> +
>> +rxm->data_off = RTE_PKTMBUF_HEADROOM;
> Incorrect indentation.

Oops... ;)

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Vlad Zolotarov]

On 03/04/15 02:33, Stephen Hemminger wrote:
> On Tue,  3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov  wrote:
>
>> +next_desc:
>> +/*
>> + * The code in this whole file uses the volatile pointer to
>> + * ensure the read ordering of the status and the rest of the
>> + * descriptor fields (on the compiler level only!!!). This is so
>> + * UGLY - why not to just use the compiler barrier instead? DPDK
>> + * even has the rte_compiler_barrier() for that.
>> + *
>> + * But most importantly this is just wrong because this doesn't
>> + * ensure memory ordering in a general case at all. For
>> + * instance, DPDK is supposed to work on Power CPUs where
>> + * compiler barrier may just not be enough!
>> + *
>> + * I tried to write only this function properly to have a
>> + * starting point (as a part of an LRO/RSC series) but the
>> + * compiler cursed at me when I tried to cast away the
>> + * "volatile" from rx_ring (yes, it's volatile too!!!). So, I'm
>> + * keeping it the way it is for now.
>> + *
>> + * The code in this file is broken in so many other places and
>> + * will just not work on a big endian CPU anyway therefore the
>> + * lines below will have to be revisited together with the rest
>> + * of the ixgbe PMD.
>> + *
>> + * TODO:
>> + *- Get rid of "volatile" crap and let the compiler do its
>> + *  job.
>> + *- Use the proper memory barrier (rte_rmb()) to ensure the
>> + *  memory ordering below.
> This comment screams "this is broken".
> Why not get proper architecture independent barriers in DPDK first.

This series is orthogonal to the issue above. I just couldn't stand to 
mention this ugliness when I noticed it on the way.
Note that although this is obviously not the right way to write this 
kind of code it is still not a bug and most likely the performance 
implications are minimal here.
The only overhead is that there may be read "too much" data from the 
descriptor that we may not actually need. The descriptor is 16 bytes so 
this doesn't seem to be a critical issue.

So, fixing the above issue may wait, especially since the same s..t may 
be found in other Intel PMDs (see i40e for example). Fixing this issue 
should be a matter of a massive cleanup series that cover all the 
relevant PMDs. Of course we may start with ixgbe but even in this single 
PMD there are at least 3 non-LRO related functions that have to be 
fixed, so IMHO even fixing ONLY ixgbe should be a matter of a separate 
series.

[dpdk-dev] [PATCH] rte_mbuf: scattered pktmbufs freeing optimization

[Olivier MATZ]

Hi Vadim,

On 02/27/2015 06:09 PM, Vadim Suraev wrote:
>  >Indeed, this function looks useful, and I also have a work in progress
>  >on this topic, but currently it is not well tested.
> I'm sorry, I didn't know. I'll not interfere with my patch))

That not what I wanted to say :)

You are very welcome with your patch, I just wanted to notify
that I am also working in the same area and that's why I listed
the things I'm currently working on.


>  >About the inlining, I have no objection now, although Stephen may be
>  >right. I think we can consider un-inline some functions, based on
>  >performance measurements.
> I've also noticed in many cases it makes no difference. Seems to be some
> trade-off.
>
>  >- clarify the difference between raw_alloc/raw_free and
>  >  mempool_get/mempool_put: For instance, I think that the reference
>  >  counter initialization should be managed by rte_pktmbuf_reset() like
>  >  the rest of the fields, therefore raw_alloc/raw_free could be replaced
> It looks useful to me since not all of the fields are used in every
> particular application so
> if the allocation is decoupled from reset, one can save some cycles.

Yes, this is also a trade-off between maintainability and speed, and
speed is probably the first constraint for the dpdk. But maybe we can
find an alternative that is both fast and maintainable.

Thanks,
Olivier

[dpdk-dev] [PATCH 7/7] bond: remove debug function to fix link with shared lib

[Declan Doherty]

On 03/03/15 15:23, Thomas Monjalon wrote:
> The function print_client_stats was used in the example without being
> clearly exported in the map file. So it breaks linking with shared library
> when debug is enabled.
> It's better to remove this function as it probably could be implemented
> with statistics API.
>
> Fixes: cc7e8ae84faa ("add example application for link bonding mode 6")
>
> Signed-off-by: Thomas Monjalon 
> ---
> 
>
Acked-by: Declan Doherty 

[dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Neil Horman]

On Wed, Mar 04, 2015 at 01:54:49PM +0100, Thomas Monjalon wrote:
> Hi Neil,
> 
> I remove parts that I agree and reply to those which deserve more discussion.
> 
> 2015-03-04 06:49, Neil Horman:
> > On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> > > 2015-02-02 13:18, Neil Horman:
> > > > +# Validate that we have all the arguments we need
> > > > +res=$(validate_args)
> > > > +if [ -n "$res" ]
> > > > +then
> > > > +   echo $res
> > > 
> > > Should be redirected to stderr >&2
> > > 
> > Why? this is eactly what I intended.  All the other messages from log are
> > directed to stdout, so should this be.
> 
> I'm wondering if there's some normal output which could be redirected for
> further processing, and some error output.
> My comment was not only for this log but also for every error message.
> 

No, the report output is in html format and always to a file, so stdout isn't
used for any inline informational reporting.

> > > I guess this is the tool:
> > >   http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> > 
> > Correct.
> 
> So maybe you should add this URL in the commit log.
> 
sure, fine.

> > > > +log "INFO" "We're going to check and make sure that applications built"
> > > > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when 
> > > > executed"
> > > > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > > > +log "INFO" ""
> 
> > > > +if [ ! -d ./.git ]
> > > > +then
> > > > +   log "WARN" "You must be in the root of the dpdk git tree"
> > > > +   log "WARN" "You are in $PWD"
> > > > +   cleanup_and_exit 1
> > > > +fi
> > > 
> > > Why not cd $(dirname $0)/.. instead of returning an error?
> > 
> > Why would that help in finding the base of the git tree.  Theres no 
> > guarantee
> > that you are in a subdirectory of a git tree.  I suppose we can try it
> > recursively until we hit /, but it seems just as easy and clear to tell the 
> > user
> > whats needed.
> 
> No I'm saying that you could avoid this check by going into the right
> directory from the beginning.
> We know that the root dir is $(dirname $0)/.. because this script is in
> scripts/ directory.
> 
That only helps if you start from the right directory.  If you run this command
from some other location, your solution just breaks.

> > > > +# Make sure we configure SHARED libraries
> > > > +# Also turn off IGB and KNI as those require kernel headers to build
> > > > +sed -i -e"$ a\CONFIG_RTE_BUILD_SHARED_LIB=y" config/defconfig_$TARGET
> > > > +sed -i -e"$ a\CONFIG_RTE_EAL_IGB_UIO=n" config/defconfig_$TARGET
> > > > +sed -i -e"$ a\CONFIG_RTE_LIBRTE_KNI=n" config/defconfig_$TARGET
> > > 
> > > Why not tuning configuration after make config in .config file?
> > > 
> > Because this way we save a reconfig (from a developer viewpoint), you 
> > should run
> > make config again after changing configs, and so this way you save doing 
> > that.
> 
> No, you run make config once and update .config file. That's the recommended
> way to configure DPDK.
> defconfig files are default configurations and should stay read-only.
They get overwritten when we do the git resets.  Its silly to modify your config
file after you run make config, in the event the make target has to re-read any
modified options and adjust dependent config files accordingly.  I understand
that doesn't happen now, but its common practice for every open source project
in existance.

> 
> > > > +for i in `ls *.so`
> > > 
> > > I think ls is useless.
> > > 
> > Um, I don't?  Not sure what you're getting at here.
> 
> I think "for i in *.so" should work.
> 
Then its irrelevant in my mind.  They both work equally well.


> > > > +   #compare the abi dumps
> > > > +   $ABICHECK -l $LIBNAME -old $ABI_DIR/$OLDNAME -new 
> > > > $ABI_DIR/$NEWNAME
> > > 
> > > Do we need to do a visual check? I didn't try yet.
> > > 
> > Yes, it generates an html report of all the symbols exported in a build and
> > compares them with the alternate version.  That needs manual review.
> 
> OK I think it's important to explain in the commit log.
Ok.

> 
> > > So you compare the ABI dumps.
> > > Do we also need to run an app from TAG2 with libs from TAG1?
> > 
> > I started down that path, but its not really that helpful, as all it will 
> > do is
> > refuse to run if there is a symbol missing from a later version.  While that
> > might be helpful, its no where near as through as the full report from the
> > compliance checker.
> > 
> > The bottom line is that real ABI compliance requires a developer to be 
> > aware of
> > the changes going into the code and how they affect binary layout. A simple
> > "does it still work" test isn't sufficient.
> 
> I hope we'll be able to integrate this kind of tool in an automated sanity
> check in order to find obvious errors.
> 
> Thanks
> 

[dpdk-dev] [PATCH v1 5/5] ixgbe: Add LRO support

[Vlad Zolotarov]

On 03/04/15 02:33, Stephen Hemminger wrote:
> On Tue,  3 Mar 2015 21:48:43 +0200
> Vlad Zolotarov  wrote:
>
>> +lro_bulk_alloc: 1, /**< RX LRO with bulk alloc is ON(1) / 
>> OFF(0) */
> This is an internal decision and should not be exposed in the API.
> We need less knobs not more.


I get your point. I'll move this to ixgbe-specific dev data.

[dpdk-dev] [PATCH v2] librte_eal/common: Fix cast from pointer to integer of different size

[Pawel Wodkowski]

On 2015-03-04 02:58, Qiu, Michael wrote:
> On 3/3/2015 9:38 PM, Wodkowski, PawelX wrote:
>>> -Original Message-
>>> From: Qiu, Michael
>>> Sent: Tuesday, March 03, 2015 11:00 AM
>>> To: Wodkowski, PawelX; dev at dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH v2] librte_eal/common: Fix cast from pointer 
>>> to
>>> integer of different size
>>>
>>> On 3/3/2015 4:25 PM, Wodkowski, PawelX wrote:
 On 2015-03-03 03:20, Michael Qiu wrote:
> /i686-native-linuxapp-gcc/include/rte_memcpy.h:592:23: error:
> cast from pointer to integer of different size
> [-Werror=pointer-to-int-cast]
>
> dstofss = 16 - (int)((long long)(void *)dst & 0x0F) + 16;
>
> Type 'long long' is 64-bit in i686 platform while 'void *'
> is 32-bit.
>
> Signed-off-by: Michael Qiu 
> ---
> v2 --> v1:
>   Remove unnecessary casting (void *)
>
>lib/librte_eal/common/include/arch/x86/rte_memcpy.h | 4 ++--
>1 file changed, 2 insertions(+), 2 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 7b2d382..85a5f4d 100644
> --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> @@ -589,12 +589,12 @@ COPY_BLOCK_64_BACK15:
>* unaligned copy functions require up to 15 bytes
>* backwards access.
>*/
> - dstofss = 16 - (int)((long long)(void *)dst & 0x0F) + 16;
> + dstofss = 16 - (int)((long)dst & 0x0F) + 16;
>   n -= dstofss;
>   rte_mov32((uint8_t *)dst, (const uint8_t *)src);
>   src = (const uint8_t *)src + dstofss;
>   dst = (uint8_t *)dst + dstofss;
> - srcofs = (int)((long long)(const void *)src & 0x0F);
> + srcofs = (int)((long)src & 0x0F);
>
>   /**
>* For aligned copy
>
 I think you should use here size_t, (u)ptrdiff_t or (u)intptr_t as this
>>> Yes, but 'long' is enough, does it limit anything, or has any issue with
>>> multiple platforms?
>>>
>> Those types ((u)ptrdiff_t, (u)intptr_t) exists specially for
>> pointer-to-int and int-to-pointer casts. Using integer primitives might
>> produce further warnings/error in the future and need further patches
>> fixing the same place.
>
> OK, I will send out the v3 patch.
>
>> Also why make offset variables signed and different type? This introduce
>> a lot of unnecessary explicit and implicit casts or type promotions.
>
> But Is it suitable to make offset (u)ptrdiff_t or (u)intptr_t?
>

I think, as final result is offset, its type should be size_t (the same 
type as type of offsetof() macro). This way you can use 
uptrdiff_t/uintptr_t and does not need of any signed-unsigned casting.

> Thanks,
> Michael
>
 will be more portable.
 Also type of dstofss and srcofs should be changed accordingly.
>>> No, I think, it should be offset.
>>>
>>> Thanks,
>>> Michael
>>
>
>


-- 
Pawel

[dpdk-dev] [PATCH] doc: prog guide cleanup for eal multi-pthread

[Cunming Liang]

Reported-by: Butler, Siobhan A 
Signed-off-by: Cunming Liang 
---
 Fixes: 1733be6d3147(doc: new eal multi-pthread feature) 

 doc/guides/prog_guide/env_abstraction_layer.rst | 82 -
 1 file changed, 40 insertions(+), 42 deletions(-)

diff --git a/doc/guides/prog_guide/env_abstraction_layer.rst 
b/doc/guides/prog_guide/env_abstraction_layer.rst
index 170deec..2f9f7ee 100644
--- a/doc/guides/prog_guide/env_abstraction_layer.rst
+++ b/doc/guides/prog_guide/env_abstraction_layer.rst
@@ -216,30 +216,31 @@ Memory zones can also be reserved from either 2 MB or 1 
GB hugepages, provided t
 Multiple pthread
 

-DPDK usually pin one pthread per core to avoid task switch overhead. It gains
-performance a lot, but it's not flexible and not always efficient.
+DPDK usually pins one pthread per core to avoid the overhead of task switching.
+This allows for significant performance gains, but lacks flexibility and is 
not always efficient.

-Power management helps to improve the cpu efficient by limiting the cpu 
runtime frequency.
-But there's more reasonable motivation to utilize the ineffective idle cycles 
under the full capability of cpu.
+Power management helps to improve the CPU efficiency by limiting the CPU 
runtime frequency.
+However, alternately it is possible to utilize the idle cycles available to 
take advantage of
+the full capability of the CPU.

-By OS scheduing and cgroup, to each pthread on specified cpu, it can simply 
assign the cpu quota.
-It gives another way to improve the cpu efficiency. But the prerequisite is to 
run DPDK execution conext from multiple pthread on one core.
-
-For flexibility, it's also useful to allow the pthread affinity not only to a 
cpu but to a cpu set.
+By taking advantage of cgroup, the CPU utilization quota can be simply 
assigned.
+This gives another way to improve the CPU efficienct, however, there is a 
prerequisite;
+DPDK must handle the context switching between multiple pthreads per core.

+For further flexibility, it is useful to set pthread affinity not only to a 
CPU but to a CPU set.

 EAL pthread and lcore Affinity
 ~~

-In terms of lcore, it stands for an EAL execution unit in the EAL pthread.
-EAL pthread indicates all the pthreads created/managed by EAL, they execute 
the tasks issued by *remote_launch*.
-In each EAL pthread, there's a TLS called *_lcore_id* for the unique 
identification.
-As EAL pthreads usually 1:1 bind to the physical cpu, *_lcore_id* typically 
equals to the cpu id.
+The term "lcore" refers to an EAL thread, which is really a Linux/FreeBSD 
pthread.
+"EAL pthreads"  are created and managed by EAL and execute the tasks issued by 
*remote_launch*.
+In each EAL pthread, there is a TLS (Thread Local Storage) called *_lcore_id* 
for unique identification.
+As EAL pthreads usually bind 1:1 to the physical CPU, the *_lcore_id* is 
typically equal to the CPU ID.

-In multiple pthread case, EAL pthread is no longer always bind to one specific 
physical cpu.
-It may affinity to a cpuset. Then the *_lcore_id* won't always be the same as 
cpu id.
-So there's an EAL long option '--lcores' defined to assign the cpu affinity of 
lcores.
-For a specified lcore id or id group, it allows to set the cpuset for that EAL 
pthread.
+When using multiple pthreads, however, the binding is no longer always 1:1 
between an EAL pthread and a specified physical CPU.
+The EAL pthread may have affinity to a CPU set, and as such the *_lcore_id* 
will not be the same as the CPU ID.
+For this reason, there is an EAL long option '--lcores' defined to assign the 
CPU affinity of lcores.
+For a specified lcore ID or ID group, the option allows setting the CPU set 
for that EAL pthread.

 The format pattern:
--lcores='[@cpu_set][,[@cpu_set],...]'
@@ -248,7 +249,7 @@ The format pattern:

 A number is a "digit([0-9]+)"; a range is "-"; a group is 
"([,,...])".

-If not supply a '\@cpu_set', the value of 'cpu_set' uses the same value as 
'lcore_set'.
+If a '\@cpu_set' value is not supplied, the value of 'cpu_set' will default to 
the value of 'lcore_set'.

 ::

@@ -261,31 +262,29 @@ If not supply a '\@cpu_set', the value of 'cpu_set' uses 
the same value as 'lcor
lcore 7 runs on cpuset 0x80 (cpu 7);
lcore 8 runs on cpuset 0x100 (cpu 8).

-By this option, for each given lcore id, the associated cpus can be assigned.
+Using this option, for each given lcore ID, the associated CPUs can be 
assigned.
 It's also compatible with the pattern of corelist('-l') option.

 non-EAL pthread support
 ~~~

-It allows to use DPDK execution context in any user pthread(aka. non-EAL 
pthread).
-
-In a non-EAL pthread, the *_lcore_id* is always LCORE_ID_ANY which means it's 
not an EAL thread along with a valid *_lcore_id*.
-Then the libraries won't take *_lcore_id* as unique id. Instead of it, some 
libraries use another alternative unique 

[dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type in rte_mbuf

[Zhang, Helin]

> -Original Message-
> From: Chilikin, Andrey
> Sent: Monday, March 2, 2015 7:48 PM
> To: Zhang, Helin; dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type in
> rte_mbuf
> 
> Hi Helin,
> 
> I see that you have removed "uint16_t reserved" member from rte_mbuf:
> 
> > +   uint16_t data_len;/**< Amount of data in segment buffer. */
> > uint16_t vlan_tci;/**< VLAN Tag Control Identifier (CPU order)
> */
> > -   uint16_t reserved;
> > union {
> > uint32_t rss; /**< RSS hash result if RSS enabled */
> 
> This reserved field was kept next to  vlan_tci as a placeholder for the second
> VLAN label for QinQ support so if need be vlan_tci + reserved could be casted 
> to
> 32 bit QinQ value or one 32bit VNTAG label. Without keeping two label adjusted
> to each other casting to 32 bit will not be possible and will affect QinQ
> performance.
Yes, but packet type is quite important which needs to be extended from 16 bits 
to 32 bits.
For FVL, the vlan tags are in different fields. We can think of putting them 
together in mbuf,
Possibly move current vlan tag down, and add one more 16 bits. Let's see what 
is the best then.
Thanks for the notes!

Regards,
Helin

> 
> Regards,
> Andrey
> 
> 
> > -Original Message-
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Helin Zhang
> > Sent: Friday, February 27, 2015 1:11 PM
> > To: dev at dpdk.org
> > Subject: [dpdk-dev] [PATCH v4 01/18] mbuf: redefinition of packet_type
> > in rte_mbuf
> >
> > In order to unify the packet type, the field of 'packet_type' in
> > 'struct rte_mbuf' needs to be extended from 16 to 32 bits.
> > Accordingly, some fields in 'struct rte_mbuf' are re-organized to
> > support this change for Vector PMD. As 'struct rte_kni_mbuf' for KNI
> > should be right mapped to 'struct rte_mbuf', it should be modified
> > accordingly. In addition, Vector PMD of ixgbe is disabled by default, as 
> > 'struct
> rte_mbuf' changed.
> >
> > Signed-off-by: Helin Zhang 
> > Signed-off-by: Cunming Liang 
> > ---
> >  config/common_linuxapp |  2 +-
> >  .../linuxapp/eal/include/exec-env/rte_kni_common.h |  4 ++--
> >  lib/librte_mbuf/rte_mbuf.h | 23
> +++---
> >  3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > v2 changes:
> > * Enlarged the packet_type field from 16 bits to 32 bits.
> > * Redefined the packet type sub-fields.
> > * Updated the 'struct rte_kni_mbuf' for KNI according to the mbuf changes.
> >
> > v3 changes:
> > * Put the mbuf layout changes into a single patch.
> > * Disabled vector ixgbe PMD by default, as mbuf layout changed.
> >
> > diff --git a/config/common_linuxapp b/config/common_linuxapp index
> > 97f1c9e..97d7bae 100644
> > --- a/config/common_linuxapp
> > +++ b/config/common_linuxapp
> > @@ -166,7 +166,7 @@ CONFIG_RTE_LIBRTE_IXGBE_DEBUG_TX_FREE=n
> >  CONFIG_RTE_LIBRTE_IXGBE_DEBUG_DRIVER=n
> >  CONFIG_RTE_LIBRTE_IXGBE_PF_DISABLE_STRIP_CRC=n
> >  CONFIG_RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC=y
> > -CONFIG_RTE_IXGBE_INC_VECTOR=y
> > +CONFIG_RTE_IXGBE_INC_VECTOR=n
> >  CONFIG_RTE_IXGBE_RX_OLFLAGS_ENABLE=y
> >
> >  #
> > diff --git
> > a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > index 1e55c2d..bd1cc09 100644
> > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h
> > @@ -117,9 +117,9 @@ struct rte_kni_mbuf {
> > uint16_t data_off;  /**< Start address of data in segment buffer. */
> > char pad1[4];
> > uint64_t ol_flags;  /**< Offload features. */
> > -   char pad2[2];
> > -   uint16_t data_len;  /**< Amount of data in segment buffer. */
> > +   char pad2[4];
> > uint32_t pkt_len;   /**< Total pkt len: sum of all segment data_len.
> > */
> > +   uint16_t data_len;  /**< Amount of data in segment buffer. */
> >
> > /* fields on second cache line */
> > char pad3[8] __attribute__((__aligned__(RTE_CACHE_LINE_SIZE)));
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 17ba791..f5b7a8b 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -258,17 +258,26 @@ struct rte_mbuf {
> > /* remaining bytes are set on RX when pulling packet from descriptor
> > */
> > MARKER rx_descriptor_fields1;
> >
> > -   /**
> > -* The packet type, which is used to indicate ordinary packet and also
> > -* tunneled packet format, i.e. each number is represented a type of
> > -* packet.
> > +   /*
> > +* The packet type, which is the combination of outer/inner L2, L3, L4
> > +* and tunnel types.
> >  */
> > -   uint16_t packet_type;
> > +   union {
> > +   uint32_t packet_type; /**< L2/L3/L4 and tunnel information.
> > */
> > +   struct {
> > +   

[dpdk-dev] [PATCH] config: default to shared library

[Panu Matilainen]

With symbol versioning its vital that developers test their code in
shared library mode, otherwise we'll be playing "add the forgotten
symbol export" from here to eternity. By defaulting to shared we
should catch more of these cases early, but without taking away anybodys
ability to build static.

Signed-off-by: Panu Matilainen 
---
 config/common_bsdapp   | 2 +-
 config/common_linuxapp | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/config/common_bsdapp b/config/common_bsdapp
index 8ff4dc2..76b97be 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -76,7 +76,7 @@ CONFIG_RTE_FORCE_INTRINSICS=n
 #
 # Compile to share library
 #
-CONFIG_RTE_BUILD_SHARED_LIB=n
+CONFIG_RTE_BUILD_SHARED_LIB=y

 #
 # Combine to one single library
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 97f1c9e..1852951 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -76,7 +76,7 @@ CONFIG_RTE_FORCE_INTRINSICS=n
 #
 # Compile to share library
 #
-CONFIG_RTE_BUILD_SHARED_LIB=n
+CONFIG_RTE_BUILD_SHARED_LIB=y

 #
 # Combine to one single library
-- 
2.1.0

[dpdk-dev] [PATCH] ethdev: add missing symbol export for rte_eth_dev_release_port

[Panu Matilainen]

Fixes: 36ec8585b298 ("ethdev: release port")

Signed-off-by: Panu Matilainen 
---
 lib/librte_ether/rte_ether_version.map | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ether/rte_ether_version.map 
b/lib/librte_ether/rte_ether_version.map
index 0d46578..a2d25a6 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -43,6 +43,7 @@ DPDK_2.0 {
rte_eth_dev_mac_addr_add;
rte_eth_dev_mac_addr_remove;
rte_eth_dev_priority_flow_ctrl_set;
+   rte_eth_dev_release_port;
rte_eth_dev_rss_hash_conf_get;
rte_eth_dev_rss_hash_update;
rte_eth_dev_rss_reta_query;
-- 
2.1.0

[dpdk-dev] [PATCH v2] ABI: Add abi checking utility

[Neil Horman]

On Tue, Mar 03, 2015 at 11:18:47PM +0100, Thomas Monjalon wrote:
> 2015-02-02 13:18, Neil Horman:
> > There was a request for an abi validation utiltyfor the ongoing ABI 
> > stability
> > work.  As it turns out there is a abi compliance checker in development that
> > seems to be under active development and provides fairly detailed ABI 
> > compliance
> > reports.  Its not yet intellegent enough to understand symbol versioning, 
> > but it
> > does provide the ability to identify symbols which have changed between
> > releases, along with details of the change, and offers develoeprs the
> > opportunity to identify which symbols then need versioning and validation 
> > for a
> > given update via manaul testing.
> 
> There's a lot of typos in this text. Please check.
> 
Three.  Theres 3 typos.  But sure, I'll fix them.

>
> > +
> > +usage() {
> > +   echo "$0   "
> > +}
> > +
> > +log() {
> > +   local level=$1
> 
> level is not used later?
> 
Not yet, but you'll note all the log calls start with a log level to add
filtering.  I'd rather leave this here as it doesn't hurt anything and
effectively documents the paramter.

>
> > +   shift
> > +   echo "$*"
> > +}
> > +
> > +validate_tags() {
> > +   git tag -l | grep -q "$TAG1"
> > +   if [ $? -ne 0 ]
> > +   then
> > +   echo "$TAG1 is invalid"
> > +   return
> > +   fi
> > +   git tag -l | grep -q "$TAG2"
> > +   if [ $? -ne 0 ]
> > +   then
> > +   echo "$TAG2 is invalid"
> > +   return
> > +   fi
> > +}
> > +
> > +validate_args() {
> > +   if [ -z "$TAG1" ]
> > +   then
> > +   echo "Must Specify TAG1"
> > +   return
> > +   fi
> > +   if [ -z "$TAG2" ]
> > +   then
> > +   echo "Must Specify TAG2"
> > +   return
> > +   fi
> > +   if [ -z "$TARGET" ]
> > +   then
> > +   echo "Must Specify a build target"
> > +   fi
> > +}
> > +
> > +
> > +cleanup_and_exit() {
> > +   rm -rf $ABI_DIR
> > +   exit $1
> > +}
> 
> This function could be automatically invoked with trap.
> 
Yes, I can add that.

> > +###
> > +#START
> > +
> > +
> > +#Save the current branch
> > +CURRENT_BRANCH=`git branch | grep \* | cut -d' ' -f2`
> 
> Will it work when not on any branch?
> 
No it won't, and I honestly wasn't that worried about it, as people
don't/shouldn't make changes in detached head state.  I can add a check to
ensure you're on a branch though.

> > +
> > +if [ -n "$VERBOSE" ]
> > +then
> > +   export VERBOSE=/dev/stdout
> > +else
> > +   export VERBOSE=/dev/null
> > +fi
> > +
> > +# Validate that we have all the arguments we need
> > +res=$(validate_args)
> > +if [ -n "$res" ]
> > +then
> > +   echo $res
> 
> Should be redirected to stderr >&2
> 
Why? this is eactly what I intended.  All the other messages from log are
directed to stdout, so should this be.

> > +   usage
> > +   cleanup_and_exit 1
> > +fi
> > +
> > +# Make sure our tags exist
> > +res=$(validate_tags)
> > +if [ -n "$res" ]
> > +then
> > +   echo $res
> > +   cleanup_and_exit 1
> > +fi
> > +
> > +ABICHECK=`which abi-compliance-checker 2>/dev/null`
> 
> Why not using the $() form like above?
> 
I don't honestly recall, but I do remember fighting trying to get output from
that format for some reason, and just left this as it was, as it wasn't
particularly relevant.

> I guess this is the tool:
>   http://ispras.linuxbase.org/index.php/ABI_compliance_checker
> 
Correct.

> > +if [ $? -ne 0 ]
> > +then
> > +   log "INFO" "Cant find abi-compliance-checker utility"
> > +   cleanup_and_exit 1
> > +fi
> > +
> > +ABIDUMP=`which abi-dumper 2>/dev/null`
> > +if [ $? -ne 0 ]
> > +then
> > +   log "INFO" "Cant find abi-dumper utility"
> > +   cleanup_and_exit 1
> > +fi
> > +
> > +log "INFO" "We're going to check and make sure that applications built"
> > +log "INFO" "against DPDK DSOs from tag $TAG1 will still run when executed"
> > +log "INFO" "against DPDK DSOs built from tag $TAG2."
> > +log "INFO" ""
> > +
> > +# Check to make sure we have a clean tree
> > +git status | grep -q clean
> > +if [ $? -ne 0 ]
> > +then
> 
> You may compact in one line:
> if git status | grep -q clean ; then
> 
I explicitly do execution and error checking on separate lines as I think its
more clear.  You'll find this style consistent in the script.

> > +   log "WARN" "Working directory not clean, aborting"
> > +   cleanup_and_exit 1
> > +fi
> > +
> > +if [ ! -d ./.git ]
> > +then
> > +   log "WARN" "You must be in the root of the dpdk git tree"
> > +   log "WARN" "You are in $PWD"
> > +   cleanup_and_exit 1
> > +fi
> 
> Why not cd $(dirname $0)/.. instead of returning an error?
> 
Why would that help in finding the base of the git tree.  Theres no guarantee
that you are in a subdirectory of a git tree.  I suppose we can try it
recursively until we hit /, but it seems just as easy and clear to tell the user
whats needed.

> > +log "INFO" "Checking out version $TAG1 of the 

[dpdk-dev] [PATCH] config: default to shared library

[Neil Horman]

On Wed, Mar 04, 2015 at 01:05:07PM +0200, Panu Matilainen wrote:
> On 03/04/2015 11:24 AM, Thomas Monjalon wrote:
> >Hi Panu,
> >
> >2015-03-04 08:17, Panu Matilainen:
> >>With symbol versioning its vital that developers test their code in
> >>shared library mode, otherwise we'll be playing "add the forgotten
> >>symbol export" from here to eternity.
> >
> >Yes we must improve the sanity checks.
> >A lot of options must be tested (or removed) and not only shared libs.
> >But the error you reported before (missing export of 
> >rte_eth_dev_release_port)
> >cannot be seen even with this patch.
> 
> I know, I didn't say it would have directly caught it. It would've likely
> been found earlier though, if nothing else then in testing of the new
> librte_pmd_null which clearly nobody had tried in shared lib configuration.
> 
This is accurate.  The default config is a tool, in the sense that it leverages
the implicit testing of any users who are experimenting with the DPDK.  Any
users out there using the DPDK test/example applications would have realized
something was amiss when the testpmd app refused to run with the null or pcap
pmd, since there was a missing symbol.  That "social fuzzing" has value, but it
only works if the defaults are carefully selected.  Currently, building for
shared libraries exposes more existing bugs than static libraries, and so we
should set that as our default so as to catch them.

> >It means we need more tools.
> >Though, default configuration is not a tool.
> 
> Yes, default config is not a tool, its a recommendation of sorts both for
> developers and users. It also tends to be the setup that is rarely broken
> because it happens to get the most testing :)
> 
And it is a tool (see above).

> >
> >>By defaulting to shared we should catch more of these cases early,
> >>but without taking away anybodys ability to build static.
> >
> >Shared libraries are convenient for distributions but have a performance
> >impact. I think that static build must remain the default choice.
> 

If utmost performance is the concern, isn't it reasonable to assume that users
in that demographic will customize their configuration to achieve that?  No one
assumes that something is tuned to be perfect for their needs out of the box if
their needs are extreemely biased to a single quality.  The best course of
action here is to set the default to be adventageous toward catching bugs, and
document the changes needed to bias for performance.

> For distros, this is not a matter of *convenience*, its the only technically
> feasible choice.
> 
> I didn't want to make the commit message into a shared library sermon, but
> if you look at the OSS landscape overall the common wisdom is that shared
> library benefits outweigh any performance impact by so much that static libs
> are almost nowhere to be found. I can change the text into a full-blown
> rationale why shared libraries should be the default if that makes any
> difference.
> 
Embedded applications actually do make extensive use of static linking to try
achieve greater performance, but they tend to be proprietary, and as such are
the exception that proves the rule.  Once an application itself becomes open
source, it biases toward shared libraries, because the minor performance impact
is well worth the increased manageability and security found in DSO's

Acked-by: Neil Horman 

>   - Panu -
> 
> 

[dpdk-dev] [PATCH] A fix to work around strict-aliasing rules breaking

[Wang, Zhihong]

> -Original Message-
> From: Wodkowski, PawelX
> Sent: Monday, March 02, 2015 8:32 PM
> To: Richardson, Bruce; Wang, Zhihong
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] A fix to work around strict-aliasing rules
> breaking
> 
> On 2015-03-02 11:32, Bruce Richardson wrote:
> > On Mon, Mar 02, 2015 at 05:03:50PM +0800, zhihong.wang at intel.com
> wrote:
> >> Fixed strict-aliasing rules breaking errors for some GCC version.
> >>
> >
> > This looks messy. Also, I believe the definition of memcpy should
> > include the "restrict" keyword to indicate that source and dest can't
> > overlap. Might that help fix the issue?
> >
> 
> Is this error related with overlapping or casting 'void *' to 'uintXX_t *' 
> that
> make compiler report aliasing rule breaking?
> 
> >
> >> Signed-off-by: Zhihong Wang 
> >> ---
> >>   .../common/include/arch/x86/rte_memcpy.h   | 44 --
> 
> >>   1 file changed, 24 insertions(+), 20 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 69a5c6f..f412099 100644
> >> --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> >> +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> >> @@ -195,6 +195,8 @@ rte_mov256blocks(uint8_t *dst, const uint8_t *src,
> size_t n)
> >>   static inline void *
> >>   rte_memcpy(void *dst, const void *src, size_t n)
> >>   {
> >> +  uintptr_t dstu = (uintptr_t)dst;
> >> +  uintptr_t srcu = (uintptr_t)src;
> 
> If so maybe using union here would be good solution or 'char *'.

Pawel,

Thanks for the suggestion! But I don't think union can work around this --- 
already tried in CentOS release 6.5.
Anyway this is for compiler ethics only, the assembly code generated will be 
the same no matter what kind of method is used.

Zhihong (John)

> 
> --
> Pawel

[dpdk-dev] [PATCH v4 0/6] Link Bonding mode 6 support (ALB)

[Jiajia, SunX]

Tested-by: Jiajia, SunX 

- Tested Commit: 8bfc4e3c4cb922d9c7e6b8934fdaffba268c7ed2
- OS: Fedora20 3.11.10-301.fc20.x86_64
- GCC: gcc version 4.8.3
- CPU: Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
- NIC: Intel Corporation 82599ES 10-Gigabit SFI/SFP+ Network Connection 
[8086:10fb]
- Target x86_64-native-linuxapp-gcc 
- Total 3 cases, 3 passed, 0 failed


* Connections ports between tester/ixia and switch and DUT
  - TESTER(Or IXIA)---SWITCH
  -   SWITCH-DUT
  - TESTER(Or IXIA)--DUT
  - portA--switch
  -switch--port0
  -switch--port1
  -switch--port2
  - portB--port3
* Connections ports between tester/ixia and switch and DUT
  - TESTER(Or IXIA)---SWITCH
  -   SWITCH-DUT
  - TESTER(Or IXIA)--DUT
  - portA-switch
  - portB-switch
  - portC-switch
  -   switch--port0
  -   switch--port1
  -   switch--port2

Test Setup#1 for Functional test


Tester has 4 ports(portA--portD), and DUT has 4 ports(port0--port3), then 
connect portA to port0, portB to port1, portC to port2, portD to port3. 


Test Setup#2 for Functional test


Tester has 2 ports(portA--portB), DUT has 4 ports(port0--port3), and a switch 
that supports IEEE 802.3ad Dynamic link aggregation, then connect port0-port2 
to the switch for dynamic link aggregation, connect portA to aggregated 
interface on the switch, connect portB to port3. 

Test Setup#3 for Functional test


Tester has 3 ports(portA--portC), DUT has 3 ports(port0--port2), and a switch, 
then connect port0-port2 to three ports on the switch, connect portA - portB to 
the other three ports on the switch. 

Test App
===

Use testpmd to test the mode 0, 1, 2, 3, 4, 5, use bond_app to test the mode 6. 
If test the mode 6, Set the tester port address to 
7.0.0.1, 7.0.0.2, 7.0.0.3. And it will be better to turn on the debug switch on 
the config:
CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB=n - deeper debug, if you want all arp and ipv4 
packets be printed 
CONFIG_RTE_LIBRTE_BOND_DEBUG_ALB_L1=n - don't print packets information, just 
collects statistics from clients, you can print these statistics with show 
command.


Test Case1: Mode 6(Adaptive load balance) Basic test
=

Verify basic commands of bond_app work well.

Use Setup#3

Now bond_app has commands of send, start, stop, show, help, quit
help:

  execute command 'help' in the bond_app, verify it will print all commands 
help info.

send:

  make sure that bond_app has been started, execute command 'send 
7.0.0.1' on server, use 
  tcpdump to listen client1 port , verify the port will receive ARP 
requests.

start:

  step1:
execute command 'stop' in the bond_app, then execute 'arping -c 1 
-I device 7.0.0.10' on the client1,
verify bond_app will not receive and reply the client1 ARP request.
  Step2:
Execute command 'start' in the bond_app, then execute 'arping -c 1 
-I device 7.0.0.10' on the client1,
Verify bond_app will receive and reply the client1 ARP request.

stop:

 continue the test of  command 'start', rerun step1.

show:

 execute command 'show' in the bond_app, verify it will print  some bond 
info: ex. active slaves etc.

quit:

 execute command 'quit' in the bond_app, verify it will terminate all 
threads and quit.

Verify bond_app can check the state of slaves, and do some actions.

Use Setup#1

Start bond_app.

Step1:

  Send 1 ARP request packet to bond0 from client1, verify it will receive 
and reply it on one slave.

Step2:

  Bring the port of client1 link down,  verify bond0 will detect the 
change of slaves, by the command
  'show' to check the state of bond0, verify there are just two slaves.

Step3:

  Send 1 ARP request packet to bond0 on bond1 from client1, verify it will 
receive nothing.


Test Case2: Mode 6(Adaptive load balance) RX test of mode6
=

Verify bond device can receive load balancing for IPV4 traffic.

Use Setup#3

Step1:

  Send ARP request packet (no VLAN header) to bond0 from each client, 
verify bond0 will choose 
  different slave to reply the request by the policy of round-robin.
  You can use the command 'arping -c 1 -I device 7.0.0.10' to server on 
each client. The device will
  need you to change it to the interface name of your client.

Step2:

  Send ARP request packet tagged with VLAN header to bond0 from each 
client, 

[dpdk-dev] [PATCH] tools/dpdk_nic_bind: Fix can't bind virtio-pci issue

[Ouyang, Changchun]

Hi,

> -Original Message-
> From: Richardson, Bruce
> Sent: Tuesday, March 3, 2015 7:44 PM
> To: Ouyang, Changchun
> Cc: dev at dpdk.org; Cao, Waterman
> Subject: Re: [PATCH] tools/dpdk_nic_bind: Fix can't bind virtio-pci issue
> 
> On Thu, Feb 26, 2015 at 12:57:49PM +0800, Ouyang Changchun wrote:
> First off, I think the title needs to be changed. How about something like:
> "dpdk_nic_bind: don't exit if an unused module is missing"
> 
> > In virtio test, on guest
> > 1. Bind virtio port to igb_uio driver; 2. Remove igb_uio module; 3.
> > Bind virtio port to virtio-pci driver, it fails and reports:
> >"Error - no supported modules are loaded"
> >
> > The tool should check the to-be-bound driver flag, if it is dpdk
> > driver(igb_uio, vfio etc), and the corresponding module is not loaded,
> > then exit, otherwise, just report a warning, and continue to bind the non-
> dpdk driver(like virtio-pci) to dev.
> 
> This is a good description of the problem. Once you change the title, I think
> an initial sentence is needed here by way of intro - such as:
> "The dpdk_nic_bind script will not allow ports to be bound or unbound if
> none of the kernel modules supported by DPDK is loaded. This patch relaxes
> this restriction by checking if a DPDK module is actually requested. The
> example below illustrates this problem:"
> 

Will update this description in v2.
Thanks

> >
> > Signed-off-by: Changchun Ouyang 
> > ---
> >  tools/dpdk_nic_bind.py | 7 +--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/dpdk_nic_bind.py b/tools/dpdk_nic_bind.py index
> > 2483056..8523f82 100755
> > --- a/tools/dpdk_nic_bind.py
> > +++ b/tools/dpdk_nic_bind.py
> > @@ -175,8 +175,11 @@ def check_modules():
> >
> >  # check if we have at least one loaded module
> >  if True not in [mod["Found"] for mod in mods] and b_flag is not None:
> > -print "Error - no supported modules are loaded"
> > -sys.exit(1)
> > +if b_flag in dpdk_drivers:
> > +print "Error - no supported modules(DPDK driver) are loaded"
> > +sys.exit(1)
> > +else:
> > +print "Warning - no supported modules(DPDK driver) are loaded"
> >
> >  # change DPDK driver list to only contain drivers that are loaded
> >  dpdk_drivers = [mod["Name"] for mod in mods if mod["Found"]]
> > --
> > 1.8.4.2
> >
> This is a worthwhile change. However, I think it could be made better:
> * If we try to bind a device to ixgbe, and ixgbe is not loaded, we get no 
> error
> from the script
> * If igb_uio is loaded and we try to bind a device to vfio which is not loaded
> again we get no error from the script.
> 
> If would be nice if instead of this checking explicitly for DPDK modules, we
> just check if the module to be bound is present and give an error if not.
>

For all these cases, they could be detected in function bind_one,
If any module exactly required can't be found/opened,  it will go to except 
branch and raise a error message like:
Error: bind failed . Cannot open.

Thanks
Changchun

[dpdk-dev] [PATCH] A fix to work around strict-aliasing rules breaking

[Wang, Zhihong]

> -Original Message-
> From: Richardson, Bruce
> Sent: Monday, March 02, 2015 6:32 PM
> To: Wang, Zhihong
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] A fix to work around strict-aliasing rules
> breaking
> 
> On Mon, Mar 02, 2015 at 05:03:50PM +0800, zhihong.wang at intel.com wrote:
> > Fixed strict-aliasing rules breaking errors for some GCC version.
> >
> 
> This looks messy. Also, I believe the definition of memcpy should include the
> "restrict" keyword to indicate that source and dest can't overlap. Might that
> help fix the issue?

It's actually caused by casting void * to multiple other pointer types.

> 
> /Bruce
> 
> > Signed-off-by: Zhihong Wang 
> > ---
> >  .../common/include/arch/x86/rte_memcpy.h   | 44 
> --
> >  1 file changed, 24 insertions(+), 20 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 69a5c6f..f412099 100644
> > --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > @@ -195,6 +195,8 @@ rte_mov256blocks(uint8_t *dst, const uint8_t *src,
> > size_t n)  static inline void *  rte_memcpy(void *dst, const void
> > *src, size_t n)  {
> > +   uintptr_t dstu = (uintptr_t)dst;
> > +   uintptr_t srcu = (uintptr_t)src;
> > void *ret = dst;
> > int dstofss;
> > int bits;
> > @@ -204,22 +206,22 @@ rte_memcpy(void *dst, const void *src, size_t n)
> >  */
> > if (n < 16) {
> > if (n & 0x01) {
> > -   *(uint8_t *)dst = *(const uint8_t *)src;
> > -   src = (const uint8_t *)src + 1;
> > -   dst = (uint8_t *)dst + 1;
> > +   *(uint8_t *)dstu = *(const uint8_t *)srcu;
> > +   srcu = (uintptr_t)((const uint8_t *)srcu + 1);
> > +   dstu = (uintptr_t)((uint8_t *)dstu + 1);
> > }
> > if (n & 0x02) {
> > -   *(uint16_t *)dst = *(const uint16_t *)src;
> > -   src = (const uint16_t *)src + 1;
> > -   dst = (uint16_t *)dst + 1;
> > +   *(uint16_t *)dstu = *(const uint16_t *)srcu;
> > +   srcu = (uintptr_t)((const uint16_t *)srcu + 1);
> > +   dstu = (uintptr_t)((uint16_t *)dstu + 1);
> > }
> > if (n & 0x04) {
> > -   *(uint32_t *)dst = *(const uint32_t *)src;
> > -   src = (const uint32_t *)src + 1;
> > -   dst = (uint32_t *)dst + 1;
> > +   *(uint32_t *)dstu = *(const uint32_t *)srcu;
> > +   srcu = (uintptr_t)((const uint32_t *)srcu + 1);
> > +   dstu = (uintptr_t)((uint32_t *)dstu + 1);
> > }
> > if (n & 0x08) {
> > -   *(uint64_t *)dst = *(const uint64_t *)src;
> > +   *(uint64_t *)dstu = *(const uint64_t *)srcu;
> > }
> > return ret;
> > }
> > @@ -458,6 +460,8 @@ static inline void *  rte_memcpy(void *dst, const
> > void *src, size_t n)  {
> > __m128i xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7,
> xmm8;
> > +   uintptr_t dstu = (uintptr_t)dst;
> > +   uintptr_t srcu = (uintptr_t)src;
> > void *ret = dst;
> > int dstofss;
> > int srcofs;
> > @@ -467,22 +471,22 @@ rte_memcpy(void *dst, const void *src, size_t n)
> >  */
> > if (n < 16) {
> > if (n & 0x01) {
> > -   *(uint8_t *)dst = *(const uint8_t *)src;
> > -   src = (const uint8_t *)src + 1;
> > -   dst = (uint8_t *)dst + 1;
> > +   *(uint8_t *)dstu = *(const uint8_t *)srcu;
> > +   srcu = (uintptr_t)((const uint8_t *)srcu + 1);
> > +   dstu = (uintptr_t)((uint8_t *)dstu + 1);
> > }
> > if (n & 0x02) {
> > -   *(uint16_t *)dst = *(const uint16_t *)src;
> > -   src = (const uint16_t *)src + 1;
> > -   dst = (uint16_t *)dst + 1;
> > +   *(uint16_t *)dstu = *(const uint16_t *)srcu;
> > +   srcu = (uintptr_t)((const uint16_t *)srcu + 1);
> > +   dstu = (uintptr_t)((uint16_t *)dstu + 1);
> > }
> > if (n & 0x04) {
> > -   *(uint32_t *)dst = *(const uint32_t *)src;
> > -   src = (const uint32_t *)src + 1;
> > -   dst = (uint32_t *)dst + 1;
> > +   *(uint32_t *)dstu = *(const uint32_t *)srcu;
> > +   srcu = (uintptr_t)((const uint32_t *)srcu + 1);
> > +   dstu = (uintptr_t)((uint32_t *)dstu + 1);
> > }
> > if (n & 0x08) {
> > -   *(uint64_t *)dst = *(const uint64_t *)src;
> > +   *(uint64_t *)dstu = *(const uint64_t *)srcu;
> > }
> > return ret;
> >  

[dpdk-dev] [PATCH v2] librte_eal/common: Fix cast from pointer to integer of different size

[Qiu, Michael]

On 3/3/2015 9:38 PM, Wodkowski, PawelX wrote:
>> -Original Message-
>> From: Qiu, Michael
>> Sent: Tuesday, March 03, 2015 11:00 AM
>> To: Wodkowski, PawelX; dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] librte_eal/common: Fix cast from pointer 
>> to
>> integer of different size
>>
>> On 3/3/2015 4:25 PM, Wodkowski, PawelX wrote:
>>> On 2015-03-03 03:20, Michael Qiu wrote:
 /i686-native-linuxapp-gcc/include/rte_memcpy.h:592:23: error:
 cast from pointer to integer of different size
 [-Werror=pointer-to-int-cast]

dstofss = 16 - (int)((long long)(void *)dst & 0x0F) + 16;

 Type 'long long' is 64-bit in i686 platform while 'void *'
 is 32-bit.

 Signed-off-by: Michael Qiu 
 ---
 v2 --> v1:
Remove unnecessary casting (void *)

   lib/librte_eal/common/include/arch/x86/rte_memcpy.h | 4 ++--
   1 file changed, 2 insertions(+), 2 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 7b2d382..85a5f4d 100644
 --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
 +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
 @@ -589,12 +589,12 @@ COPY_BLOCK_64_BACK15:
 * unaligned copy functions require up to 15 bytes
 * backwards access.
 */
 -  dstofss = 16 - (int)((long long)(void *)dst & 0x0F) + 16;
 +  dstofss = 16 - (int)((long)dst & 0x0F) + 16;
n -= dstofss;
rte_mov32((uint8_t *)dst, (const uint8_t *)src);
src = (const uint8_t *)src + dstofss;
dst = (uint8_t *)dst + dstofss;
 -  srcofs = (int)((long long)(const void *)src & 0x0F);
 +  srcofs = (int)((long)src & 0x0F);

/**
 * For aligned copy

>>> I think you should use here size_t, (u)ptrdiff_t or (u)intptr_t as this
>> Yes, but 'long' is enough, does it limit anything, or has any issue with
>> multiple platforms?
>>
> Those types ((u)ptrdiff_t, (u)intptr_t) exists specially for 
> pointer-to-int and int-to-pointer casts. Using integer primitives might 
> produce further warnings/error in the future and need further patches 
> fixing the same place.

OK, I will send out the v3 patch.

> Also why make offset variables signed and different type? This introduce 
> a lot of unnecessary explicit and implicit casts or type promotions.

But Is it suitable to make offset (u)ptrdiff_t or (u)intptr_t?

Thanks,
Michael

>>> will be more portable.
>>> Also type of dstofss and srcofs should be changed accordingly.
>> No, I think, it should be offset.
>>
>> Thanks,
>> Michael
>

[dpdk-dev] [PATCH 4/7] virtio: fix build with debug enabled

[Ouyang, Changchun]

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, March 3, 2015 11:24 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH 4/7] virtio: fix build with debug enabled
> 
> With CONFIG_RTE_LIBRTE_VIRTIO_DEBUG_INIT=y:
>   error: ?devname? undeclared (first use in this function)
> 
> Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")
> 
> Signed-off-by: Thomas Monjalon 

Acked-by: Changchun Ouyang 

[dpdk-dev] [PATCH 3/7] virtio: fix build with mempool debug enabled

[Ouyang, Changchun]

> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, March 3, 2015 11:24 PM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH 3/7] virtio: fix build with mempool debug
> enabled
> 
> The mempool header forces error on -Wcast-qual:
>   error: cast discards ?const? qualifier from pointer target type
> 
> Let's fix it by removing const qualifier of pci driver from commit
>   5e9f6d1340ff ("pci: reference driver structure for each device") It's
> needed because the driver flags are changed depending on using uio or not.
> Actually these driver flags should be directly attached to each device.
> 
> Fixes: da978dfdc43b ("virtio: use port IO to get PCI resource")
> 
> Signed-off-by: Thomas Monjalon 

Acked-by: Changchun Ouyang