[dpdk-dev] [RFC 0/4] Use Google Test as DPDK unit test framework

2016-08-02 Thread Thomas Monjalon
2016-08-02 21:37, Declan Doherty:
> I've been trying out using google test as a possible replacement for our unit
> test framework and have put to together this series of patches with help from
> Anatoly as RFC to get peoples thoughts on migrating to google test.

Thanks for exploring new possibilities.

> To facilitate google test this rfc patch set contains build system changes to
> allow C++ to built within the DPDK framework, this intended for unit test code
> only, and to support google test which is a C++ framework. Don't worry, I'm 
> not
> advocating making DPDK a C++ project :) 

You are not advocating but the unit test must be written in C++.
I don't think it is a good idea to force people to write and maintain the tests
in a different language than the code it tests.

> Some of the major advantages of google test that I see over continuing to use 
> the
> current test include giving a consist feel to all tests, a powerful test
> execution framework which allow individual test suites or tests to be 
> specified
> from the command line, support for a standard xunit output which can be 
> integrated
> into a continuous build systems, and a very powerful mocking library
> which allows much more control over testing failure conditions.

It would be interesting to better describe in details what is missing currently
and what such a framework can bring.
(I agree there is a huge room for improvements on unit tests)


[dpdk-dev] [PATCH v2] lpm: remove redundant check when adding lpm rule

2016-08-02 Thread Thomas Monjalon
2016-08-02 17:04, Bruce Richardson:
> Having to make this change twice shows up the fact that we are still carrying
> around some version changes for older releases. Given that we are now past the
> 16.07 release, the old code can probably be removed. Any volunteers to maybe
> do up a patch for that.

The first step would be to announce an ABI breakage.
Do you plan to do other breaking changes? We may try to group them.



[dpdk-dev] [RFC 4/4] app/test-gtest: example google test application

2016-08-02 Thread Declan Doherty
Adds a new application to demonstrate how google test could
be used as a new unit test framework.

To compile both ${GTEST_DIR} and ${GMOCK_DIR} the install paths to google
test and google mock directories respectively must be defined/exported.

Signed-off-by: Declan Doherty 
---
 app/Makefile|   1 +
 app/test-gtest/Makefile |  67 ++
 app/test-gtest/main.cpp |  20 +++
 app/test-gtest/test_mempool.cpp | 281 
 config/common_base  |   5 +
 5 files changed, 374 insertions(+)
 create mode 100644 app/test-gtest/Makefile
 create mode 100644 app/test-gtest/main.cpp
 create mode 100644 app/test-gtest/test_mempool.cpp

diff --git a/app/Makefile b/app/Makefile
index 30ec292..71cf31e 100644
--- a/app/Makefile
+++ b/app/Makefile
@@ -38,5 +38,6 @@ DIRS-$(CONFIG_RTE_TEST_PMD) += test-pmd
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += cmdline_test
 DIRS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += proc_info
 DIRS-$(CONFIG_RTE_LIBRTE_PDUMP) += pdump
+DIRS-$(CONFIG_RTE_TEST_GTEST) += test-gtest

 include $(RTE_SDK)/mk/rte.subdir.mk
diff --git a/app/test-gtest/Makefile b/app/test-gtest/Makefile
new file mode 100644
index 000..66dbe15
--- /dev/null
+++ b/app/test-gtest/Makefile
@@ -0,0 +1,67 @@
+#   BSD LICENSE
+#   BSD LICENSE
+#
+#   Copyright(c) 2016 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.
+
+
+ifneq ($(MAKECMDGOALS),clean)
+ifeq ($(GTEST_DIR),)
+$(error "Please define GTEST_DIR environment variable")
+endif
+
+ifeq ($(GMOCK_DIR),)
+$(error "Please define GMOCK_DIR environment variable")
+endif
+endif
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+# binary name
+APP = test-gtest
+
+# all source are stored in SRCS-y
+SRCS-y := main.cpp
+SRCS-y += test_mempool.cpp
+
+CFLAGS += -O0
+CFLAGS += $(WERROR_FLAGS)
+
+CXXFLAGS += -std=c++11
+CXXFLAGS += -I$(GTEST_DIR)/include 
+CXXFLAGS += -I$(GMOCK_DIR)/include
+
+LDFLAGS += -lstdc++
+LDFLAGS += -L$(GMOCK_DIR) -lgmock
+
+# this application needs libraries first
+DEPDIRS-y += lib drivers
+
+include $(RTE_SDK)/mk/rte.app.mk
+
diff --git a/app/test-gtest/main.cpp b/app/test-gtest/main.cpp
new file mode 100644
index 000..a221823
--- /dev/null
+++ b/app/test-gtest/main.cpp
@@ -0,0 +1,20 @@
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+
+int main(int argc, char *argv[])
+{
+   /* Initialise DPDK EAL */
+   int ret = rte_eal_init(argc, argv);
+   if (ret < 0)
+   rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n");
+   argc -= ret;
+   argv += ret;
+
+   testing::InitGoogleMock(&argc, argv);
+   return RUN_ALL_TESTS();
+}  
diff --git a/app/test-gtest/test_mempool.cpp b/app/test-gtest/test_mempool.cpp
new file mode 100644
index 000..4c3f7e2
--- /dev/null
+++ b/app/test-gtest/test_mempool.cpp
@@ -0,0 +1,281 @@
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+class MemoryPool : public ::testing::Test {
+
+protected:
+
+   virtual void SetUp() {
+   mp = NULL;
+   mempool_elements = (rte_lcore_count() * (lcore_cache_size +
+   RTE_MEMPOOL_CACHE_MAX_SIZE) -1);
+   }
+
+   virtual void TearDown() {
+   if (mp)
+   rte_mempool_free(mp);
+
+   }
+
+   /*
+* save the object number in the first 4 bytes of object data. All
+* other bytes are set

[dpdk-dev] [RFC 3/4] eal: add command line option to log output to stdout

2016-08-02 Thread Declan Doherty
Adds new command line options which allows the user to stop
application echoing log output to stdout, logs are still
written to syslog.

Signed-off-by: Declan Doherty 
---
 lib/librte_eal/common/eal_common_log.c | 14 ++
 lib/librte_eal/common/eal_common_options.c |  8 
 lib/librte_eal/common/eal_options.h|  2 ++
 lib/librte_eal/common/include/rte_log.h| 15 +++
 lib/librte_eal/linuxapp/eal/eal.c  |  1 +
 lib/librte_eal/linuxapp/eal/eal_log.c  | 23 ++-
 6 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_log.c 
b/lib/librte_eal/common/eal_common_log.c
index 7916c78..8319823 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -45,6 +45,7 @@
 struct rte_logs rte_logs = {
.type = ~0,
.level = RTE_LOG_DEBUG,
+   .silent = 1,
.file = NULL,
 };

@@ -102,6 +103,18 @@ rte_get_log_level(void)
return rte_logs.level;
 }

+void
+rte_log_silence_stdout(void)
+{
+   rte_logs.silent = 0;
+}
+
+int
+rte_log_stdout(void)
+{
+   return rte_logs.silent;
+}
+
 /* Set global log type */
 void
 rte_set_log_type(uint32_t type, int enable)
@@ -125,6 +138,7 @@ int rte_log_cur_msg_loglevel(void)
return RTE_PER_LCORE(log_cur_msg).loglevel;
 }

+
 /* get the current logtype for the message beeing processed */
 int rte_log_cur_msg_logtype(void)
 {
diff --git a/lib/librte_eal/common/eal_common_options.c 
b/lib/librte_eal/common/eal_common_options.c
index 1a1bab3..186cfeb 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -69,6 +69,7 @@ eal_short_options[] =
"r:" /* memory ranks */
"v"  /* version */
"w:" /* pci-whitelist */
+   "s"  /* silence log output to stdout */
;

 const struct option
@@ -95,6 +96,7 @@ eal_long_options[] = {
{OPT_VFIO_INTR, 1, NULL, OPT_VFIO_INTR_NUM},
{OPT_VMWARE_TSC_MAP,0, NULL, OPT_VMWARE_TSC_MAP_NUM   },
{OPT_XEN_DOM0,  0, NULL, OPT_XEN_DOM0_NUM },
+   {OPT_SILENT_STDOUT, 0, NULL, OPT_SILENT_STDOUT_NUM},
{0, 0, NULL, 0}
 };

@@ -842,6 +844,10 @@ eal_parse_common_option(int opt, const char *optarg,
RTE_LOG(CRIT, EAL, "RTE Version: '%s'\n", rte_version());
break;

+   case OPT_SILENT_STDOUT_NUM:
+   rte_log_silence_stdout();
+   break;
+
/* long options */
case OPT_HUGE_UNLINK_NUM:
conf->hugepage_unlink = 1;
@@ -906,6 +912,7 @@ eal_parse_common_option(int opt, const char *optarg,
conf->log_level = log;
break;
}
+
case OPT_LCORES_NUM:
if (eal_parse_lcores(optarg) < 0) {
RTE_LOG(ERR, EAL, "invalid parameter for --"
@@ -1028,6 +1035,7 @@ eal_common_usage(void)
   "  --"OPT_PROC_TYPE" Type of this process 
(primary|secondary|auto)\n"
   "  --"OPT_SYSLOG"Set syslog facility\n"
   "  --"OPT_LOG_LEVEL" Set default log level\n"
+  "  -s, --"OPT_SILENT_STDOUT" Silent mode, supress output to 
STDOUT\n"
   "  -v  Display version information on startup\n"
   "  -h, --help  This help\n"
   "\nEAL options for DEBUG use only:\n"
diff --git a/lib/librte_eal/common/eal_options.h 
b/lib/librte_eal/common/eal_options.h
index a881c62..ce6547c 100644
--- a/lib/librte_eal/common/eal_options.h
+++ b/lib/librte_eal/common/eal_options.h
@@ -41,6 +41,8 @@ enum {
OPT_PCI_BLACKLIST_NUM   = 'b',
 #define OPT_PCI_WHITELIST "pci-whitelist"
OPT_PCI_WHITELIST_NUM   = 'w',
+#define OPT_SILENT_STDOUT "log-stdout-silent"
+   OPT_SILENT_STDOUT_NUM   = 's',

/* first long only option value must be >= 256, so that we won't
 * conflict with short options */
diff --git a/lib/librte_eal/common/include/rte_log.h 
b/lib/librte_eal/common/include/rte_log.h
index b1add04..74acb91 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -56,6 +56,7 @@ extern "C" {
 struct rte_logs {
uint32_t type;  /**< Bitfield with enabled logs. */
uint32_t level; /**< Log level. */
+   uint32_t silent:1; /**< Silent mode - Don't print to STDOUT */
FILE *file; /**< Pointer to current FILE* for logs. */
 };

@@ -181,6 +182,20 @@ int rte_log_cur_msg_loglevel(void);
 int rte_log_cur_msg_logtype(void);

 /**
+ * Silence output to stdout by logging facilities
+ */
+void rte_log_silence_stdout(void);
+
+/**
+ * Check if echoing log output to stdout is enabled.
+ *
+ * @return
+ * - Returns 1 if echoing to stdout is enabled
+ * - Returns 0 if logging is in silent mode
+ */
+int rte_log_stdout(void);
+
+/**
  * @depreca

[dpdk-dev] [RFC 2/4] examples: add c++ example application

2016-08-02 Thread Declan Doherty
From: Anatoly Burakov 

Example application for C++ compilation

Signed-off-by: Declan Doherty 
---
 examples/Makefile|  1 +
 examples/helloworld-cpp/Makefile | 51 +
 examples/helloworld-cpp/c_code.c |  5 ++
 examples/helloworld-cpp/c_header.h   |  6 +++
 examples/helloworld-cpp/cpp_code.cpp | 89 
 5 files changed, 152 insertions(+)
 create mode 100644 examples/helloworld-cpp/Makefile
 create mode 100644 examples/helloworld-cpp/c_code.c
 create mode 100644 examples/helloworld-cpp/c_header.h
 create mode 100644 examples/helloworld-cpp/cpp_code.cpp

diff --git a/examples/Makefile b/examples/Makefile
index 18b41b9..cb81c6c 100644
--- a/examples/Makefile
+++ b/examples/Makefile
@@ -46,6 +46,7 @@ endif
 DIRS-y += ethtool
 DIRS-y += exception_path
 DIRS-y += helloworld
+DIRS-y += helloworld-cpp
 DIRS-$(CONFIG_RTE_LIBRTE_PIPELINE) += ip_pipeline
 ifeq ($(CONFIG_RTE_LIBRTE_LPM),y)
 DIRS-$(CONFIG_RTE_IP_FRAG) += ip_reassembly
diff --git a/examples/helloworld-cpp/Makefile b/examples/helloworld-cpp/Makefile
new file mode 100644
index 000..f8f281c
--- /dev/null
+++ b/examples/helloworld-cpp/Makefile
@@ -0,0 +1,51 @@
+#   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.
+
+ifeq ($(RTE_SDK),)
+$(error "Please define RTE_SDK environment variable")
+endif
+
+# Default target, can be overriden by command line or environment
+RTE_TARGET ?= x86_64-native-linuxapp-gcc
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+# binary name
+APP = helloworld-cpp
+
+# all source are stored in SRCS-y
+SRCS-y := c_code.c cpp_code.cpp
+
+CFLAGS += -O3
+CFLAGS += $(WERROR_FLAGS)
+LDFLAGS += -lstdc++
+
+include $(RTE_SDK)/mk/rte.extapp.mk
diff --git a/examples/helloworld-cpp/c_code.c b/examples/helloworld-cpp/c_code.c
new file mode 100644
index 000..d349a3b
--- /dev/null
+++ b/examples/helloworld-cpp/c_code.c
@@ -0,0 +1,5 @@
+#include "c_header.h"
+
+int func(void) {
+   return 100500;
+}
diff --git a/examples/helloworld-cpp/c_header.h 
b/examples/helloworld-cpp/c_header.h
new file mode 100644
index 000..3668b60
--- /dev/null
+++ b/examples/helloworld-cpp/c_header.h
@@ -0,0 +1,6 @@
+#ifndef HEADER_H
+#define HEADER_H
+
+int func(void);
+
+#endif
diff --git a/examples/helloworld-cpp/cpp_code.cpp 
b/examples/helloworld-cpp/cpp_code.cpp
new file mode 100644
index 000..6919048
--- /dev/null
+++ b/examples/helloworld-cpp/cpp_code.cpp
@@ -0,0 +1,89 @@
+/*-
+ *   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 

[dpdk-dev] [RFC 1/4] mk: Add support for C++ compilation

2016-08-02 Thread Declan Doherty
From: Anatoly Burakov 

Adding support for compiling C++ files as part of the build system

Signed-off-by: Declan Doherty 
---
 mk/internal/rte.compile-pre.mk | 52 ++
 mk/target/generic/rte.vars.mk  |  3 +++
 mk/toolchain/clang/rte.vars.mk | 14 +---
 mk/toolchain/gcc/rte.vars.mk   | 14 +---
 mk/toolchain/icc/rte.vars.mk   | 10 +---
 5 files changed, 80 insertions(+), 13 deletions(-)

diff --git a/mk/internal/rte.compile-pre.mk b/mk/internal/rte.compile-pre.mk
index f740179..2342942 100644
--- a/mk/internal/rte.compile-pre.mk
+++ b/mk/internal/rte.compile-pre.mk
@@ -32,12 +32,12 @@
 #
 # Common to rte.lib.mk, rte.app.mk, rte.obj.mk
 #
-
 SRCS-all := $(SRCS-y) $(SRCS-n) $(SRCS-)

 # convert source to obj file
 src2obj = $(strip $(patsubst %.c,%.o,\
-   $(patsubst %.S,%_s.o,$(1
+   $(patsubst %.S,%_s.o,\
+   $(patsubst %.cpp,%_cpp.o,$(1)

 # add a dot in front of the file name
 dotfile = $(strip $(foreach f,$(1),\
@@ -46,12 +46,14 @@ dotfile = $(strip $(foreach f,$(1),\
 # convert source/obj files into dot-dep filename (does not
 # include .S files)
 src2dep = $(strip $(call dotfile,$(patsubst %.c,%.o.d, \
-   $(patsubst %.S,,$(1)
+   $(patsubst %.cpp,%_cpp.o.d, \
+   $(patsubst %.S,,$(1))
 obj2dep = $(strip $(call dotfile,$(patsubst %.o,%.o.d,$(1

 # convert source/obj files into dot-cmd filename
 src2cmd = $(strip $(call dotfile,$(patsubst %.c,%.o.cmd, \
-   $(patsubst %.S,%_s.o.cmd,$(1)
+   $(patsubst %.cpp,%_cpp.o.cmd, \
+   $(patsubst %.S,%_s.o.cmd,$(1))
 obj2cmd = $(strip $(call dotfile,$(patsubst %.o,%.o.cmd,$(1

 OBJS-y := $(call src2obj,$(SRCS-y))
@@ -186,3 +188,45 @@ S_TO_O_DO = @set -e; \
$(depfile_missing),\
$(depfile_newer)),\
$(S_TO_O_DO))
+
+
+# command to compile a .cpp file to generate an object
+ifeq ($(USE_HOST),1)
+CXX_TO_O = $(HOSTCXX) -Wp,-MD,$(call obj2dep,$(@)).tmp $(HOST_CXXFLAGS) \
+   $(CXXFLAGS_$(@)) $(HOST_EXTRA_CXXFLAGS) -o $@ -c $<
+CXX_TO_O_STR = $(subst ','\'',$(CXX_TO_O)) #'# fix syntax highlight
+CXX_TO_O_DISP = $(if $(V),"$(CXX_TO_O_STR)","  HOSTCXX $(@)")
+else
+CXX_TO_O = $(CXX) -Wp,-MD,$(call obj2dep,$(@)).tmp $(CXXFLAGS) \
+   $(CXXFLAGS_$(@)) $(EXTRA_CXXFLAGS) -o $@ -c $<
+CXX_TO_O_STR = $(subst ','\'',$(CXX_TO_O)) #'# fix syntax highlight
+CXX_TO_O_DISP = $(if $(V),"$(CXX_TO_O_STR)","  CXX $(@)")
+endif
+CXX_TO_O_CMD = 'cmd_$@ = $(CXX_TO_O_STR)'
+CXX_TO_O_DO = @set -e; \
+   echo $(CXX_TO_O_DISP); \
+   $(CXX_TO_O) && \
+   echo $(CXX_TO_O_CMD) > $(call obj2cmd,$(@)) && \
+   sed 's,'$@':,dep_'$@' =,' $(call obj2dep,$(@)).tmp > $(call 
obj2dep,$(@)) && \
+   rm -f $(call obj2dep,$(@)).tmp
+
+#
+# Compile .cpp file if needed
+# Note: dep_$$@ is from the .d file and DEP_$$@ can be specified by
+# user (by default it is empty)
+#
+.SECONDEXPANSION:
+%_cpp.o: %.cpp $$(wildcard $$(dep_$$@)) $$(DEP_$$(@)) FORCE
+   @[ -d $(dir $@) ] || mkdir -p $(dir $@)
+   $(if $(D),\
+   @echo -n "$< -> $@ " ; \
+   echo -n "file_missing=$(call boolean,$(file_missing)) " ; \
+   echo -n "cmdline_changed=$(call boolean,$(call 
cmdline_changed,$(CXX_TO_O))) " ; \
+   echo -n "depfile_missing=$(call boolean,$(depfile_missing)) " ; 
\
+   echo "depfile_newer=$(call boolean,$(depfile_newer))")
+   $(if $(or \
+   $(file_missing),\
+   $(call cmdline_changed,$(CXX_TO_O)),\
+   $(depfile_missing),\
+   $(depfile_newer)),\
+   $(CXX_TO_O_DO))
\ No newline at end of file
diff --git a/mk/target/generic/rte.vars.mk b/mk/target/generic/rte.vars.mk
index 75a616a..128d4e2 100644
--- a/mk/target/generic/rte.vars.mk
+++ b/mk/target/generic/rte.vars.mk
@@ -138,7 +138,10 @@ endif
 LDFLAGS += -L$(RTE_SDK_BIN)/lib
 endif

+CXXFLAGS := $(CFLAGS)
+
 export CFLAGS
+export CXXFLAGS
 export LDFLAGS

 endif
diff --git a/mk/toolchain/clang/rte.vars.mk b/mk/toolchain/clang/rte.vars.mk
index 7749b99..2a7105e 100644
--- a/mk/toolchain/clang/rte.vars.mk
+++ b/mk/toolchain/clang/rte.vars.mk
@@ -32,8 +32,9 @@
 #
 # toolchain:
 #
-#   - define CC, LD, AR, AS, ... (overriden by cmdline value)
+#   - define CC, CXX, LD, AR, AS, ... (overriden by cmdline value)
 #   - define TOOLCHAIN_CFLAGS variable (overriden by cmdline value)
+#   - define TOOLCHAIN_CXXFLAGS variable (overriden by cmdline value)
 #   - define TOOLCHAIN_LDFLAGS variable (overriden by cmdline value)
 #   - define TOOLCHAIN_ASFLAGS variable (overriden by cmdline value)
 #
@@ -41,6 +42,7 @@
 CC= $(CROSS)clang
 KERNELCC  = $(CROSS)gcc
 CPP   = $(CROSS)cpp
+CXX   = $(CROSS)clang++
 # for now, we don't use as but nasm.
 # AS  = $(CROSS)as
 AS= nasm
@@ -57,10 +59,16 @@ HOSTCC= $(CC)
 else
 HOSTCC= clang
 endif
+ifeq ("$(origin CXX)", "command line")

[dpdk-dev] [RFC 0/4] Use Google Test as DPDK unit test framework

2016-08-02 Thread Declan Doherty
I've been trying out using google test as a possible replacement for our unit
test framework and have put to together this series of patches with help from
Anatoly as RFC to get peoples thoughts on migrating to google test.

To facilitate google test this rfc patch set contains build system changes to
allow C++ to built within the DPDK framework, this intended for unit test code
only, and to support google test which is a C++ framework. Don't worry, I'm not
advocating making DPDK a C++ project :) 

A new EAL option to allow the user to silence logging to stdout while still
logging to syslog. This was to facilitate a eclipse plugin I was using which
parses googe test output, so it is not strictly necessary.

A example application which uses the C++ build changes.

And finally a google test application which some mempool tests ported from the
current test app to give a feel of what google test is like to use for testing
C. 

Some of the major advantages of google test that I see over continuing to use 
the
current test include giving a consist feel to all tests, a powerful test
execution framework which allow individual test suites or tests to be specified
from the command line, support for a standard xunit output which can be 
integrated
into a continuous build systems, and a very powerful mocking library
which allows much more control over testing failure conditions.

Have a look at the patch set keeping in mind that this isn't intended to be
production code, I imagine that build systems changes for C++ would require some
refinement etc

Regards
Declan


Anatoly Burakov (2):
  mk: Add support for C++ compilation
  examples: add c++ example application

Declan Doherty (2):
  eal: add command line option to log output to stdout
  app/test-gtest: example google test application

 app/Makefile   |   1 +
 app/test-gtest/Makefile|  67 +++
 app/test-gtest/main.cpp|  20 ++
 app/test-gtest/test_mempool.cpp| 281 +
 config/common_base |   5 +
 examples/Makefile  |   1 +
 examples/helloworld-cpp/Makefile   |  51 ++
 examples/helloworld-cpp/c_code.c   |   5 +
 examples/helloworld-cpp/c_header.h |   6 +
 examples/helloworld-cpp/cpp_code.cpp   |  89 +
 lib/librte_eal/common/eal_common_log.c |  14 ++
 lib/librte_eal/common/eal_common_options.c |   8 +
 lib/librte_eal/common/eal_options.h|   2 +
 lib/librte_eal/common/include/rte_log.h|  15 ++
 lib/librte_eal/linuxapp/eal/eal.c  |   1 +
 lib/librte_eal/linuxapp/eal/eal_log.c  |  23 ++-
 mk/internal/rte.compile-pre.mk |  52 +-
 mk/target/generic/rte.vars.mk  |   3 +
 mk/toolchain/clang/rte.vars.mk |  14 +-
 mk/toolchain/gcc/rte.vars.mk   |  14 +-
 mk/toolchain/icc/rte.vars.mk   |  10 +-
 21 files changed, 660 insertions(+), 22 deletions(-)
 create mode 100644 app/test-gtest/Makefile
 create mode 100644 app/test-gtest/main.cpp
 create mode 100644 app/test-gtest/test_mempool.cpp
 create mode 100644 examples/helloworld-cpp/Makefile
 create mode 100644 examples/helloworld-cpp/c_code.c
 create mode 100644 examples/helloworld-cpp/c_header.h
 create mode 100644 examples/helloworld-cpp/cpp_code.cpp

-- 
2.5.5



[dpdk-dev] [PATCH v3] net/mlx5: fix possible NULL deref in Rx path

2016-08-02 Thread Sagi Grimberg
The user is allowed to call ->rx_pkt_burst() even without free
mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
on the first iteration (where pkt is still NULL). This would cause us
to deref a NULL pkt (reset refcount and free).

Fix this by checking the pkt before freeing it.

Fixes: a1bdb71a32da ("net/mlx5: fix crash in Rx")

Signed-off-by: Sagi Grimberg 
Acked-by: Adrien Mazarguil 
---
Changes from v2:
- fix check-git-log.sh complaints
- collected acked-by tag

Changes from v1:
- check pkt only once in case we failed to allocate a buffer

 drivers/net/mlx5/mlx5_rxtx.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index fce3381ae87a..37573668e43e 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, 
uint16_t pkts_n)
rte_prefetch0(wqe);
rep = rte_mbuf_raw_alloc(rxq->mp);
if (unlikely(rep == NULL)) {
+   ++rxq->stats.rx_nombuf;
+   if (!pkt) {
+   /*
+* no buffers before we even started,
+* bail out silently.
+*/
+   break;
+   }
while (pkt != seg) {
assert(pkt != (*rxq->elts)[idx]);
seg = NEXT(pkt);
@@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, 
uint16_t pkts_n)
__rte_mbuf_raw_free(pkt);
pkt = seg;
}
-   ++rxq->stats.rx_nombuf;
break;
}
if (!pkt) {
-- 
1.9.1



[dpdk-dev] [RFC] scripts: make load-devel-config not to appear as executable

2016-08-02 Thread Thomas Monjalon
2016-08-02 15:54, Christian Ehrhardt:
> Quoting the first line of the script: "#! /bin/echo must be loaded with ."
> Given that we should drop the .sh file ending as well as the executable
> flag - both are not needed to source the file.

Hmmm, it is still a file containing some shell commands, right?
So why removing the .sh extension?


[dpdk-dev] [PATCH v2] lpm: remove redundant check when adding lpm rule

2016-08-02 Thread Bruce Richardson
On Tue, Aug 02, 2016 at 10:09:25AM +0800, Wei Dai wrote:
> When a rule with depth > 24 is added into an existing
> rule with depth <=24, a new tbl8 is allocated, the existing
> rule first fulfill whole new tbl8, so the filed vaild of

typo "valid"

> each entry in this tbl8 is always true and depth of each
> entry is always < 24 before adding new rule with depth > 24.
> 
> Signed-off-by: Wei Dai 

Acked-by: Bruce Richardson 

Having to make this change twice shows up the fact that we are still carrying
around some version changes for older releases. Given that we are now past the
16.07 release, the old code can probably be removed. Any volunteers to maybe
do up a patch for that.

Regards,
/Bruce



[dpdk-dev] [PATCH 2/2] app/test: add a case to verify lpm tlb8 recycle

2016-08-02 Thread Bruce Richardson
On Mon, Aug 01, 2016 at 03:03:44PM +0800, Wei Dai wrote:
> As a bug-fix for lpm tlb8 recycle is introduced,
> add a test case to verify tlb8 group is correctly
> freed when it only includes a rule with depth=24.
> 
> Signed-off-by: Wei Dai 

Acked-by: Bruce Richardson 



[dpdk-dev] [PATCH 1/2] lpm: fix tlb8 only not freed for depth=24

2016-08-02 Thread Bruce Richardson
On Mon, Aug 01, 2016 at 03:03:20PM +0800, Wei Dai wrote:
> When all rules with depth > 24 are deleted in a same tlb8 group
> and only leave a rule with depth <= 24 in this group, the tlb8
> group should be recycled.
> 

The title needs a bit of rewording, I think e.g. 
lpm: fix freeing unused sub-table on rule delete

Otherwise:
Acked-by: Bruce Richardson 



[dpdk-dev] [RFC] scripts: make load-devel-config not to appear as executable

2016-08-02 Thread Christian Ehrhardt
Quoting the first line of the script: "#! /bin/echo must be loaded with ."
Given that we should drop the .sh file ending as well as the executable
flag - both are not needed to source the file.

Signed-off-by: Christian Ehrhardt 
---
 MAINTAINERS  |  2 +-
 scripts/checkpatches.sh  |  2 +-
 scripts/load-devel-config| 14 ++
 scripts/load-devel-config.sh | 14 --
 scripts/test-build.sh|  4 ++--
 5 files changed, 18 insertions(+), 18 deletions(-)
 create mode 100644 scripts/load-devel-config
 delete mode 100755 scripts/load-devel-config.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index 6536c6b..f8b99ee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -27,7 +27,7 @@ F: MAINTAINERS
 F: scripts/check-maintainers.sh
 F: scripts/check-git-log.sh
 F: scripts/checkpatches.sh
-F: scripts/load-devel-config.sh
+F: scripts/load-devel-config
 F: scripts/test-build.sh

 Stable Branches
diff --git a/scripts/checkpatches.sh b/scripts/checkpatches.sh
index 7111558..b596b4e 100755
--- a/scripts/checkpatches.sh
+++ b/scripts/checkpatches.sh
@@ -33,7 +33,7 @@
 # Load config options:
 # - DPDK_CHECKPATCH_PATH
 # - DPDK_CHECKPATCH_LINE_LENGTH
-. $(dirname $(readlink -e $0))/load-devel-config.sh
+. $(dirname $(readlink -e $0))/load-devel-config

 length=${DPDK_CHECKPATCH_LINE_LENGTH:-80}

diff --git a/scripts/load-devel-config b/scripts/load-devel-config
new file mode 100644
index 000..489f007
--- /dev/null
+++ b/scripts/load-devel-config
@@ -0,0 +1,14 @@
+#! /bin/echo must be loaded with .
+
+# Load DPDK devel config and allow override
+# from system file
+test ! -r /etc/dpdk/devel.config ||
+. /etc/dpdk/devel.config
+# from user file
+test ! -r ~/.config/dpdk/devel.config ||
+. ~/.config/dpdk/devel.config
+# from local file
+test ! -r $(dirname $(readlink -m $0))/../.develconfig ||
+. $(dirname $(readlink -m $0))/../.develconfig
+
+# The config files must export variables in the shell style
diff --git a/scripts/load-devel-config.sh b/scripts/load-devel-config.sh
deleted file mode 100755
index 489f007..000
--- a/scripts/load-devel-config.sh
+++ /dev/null
@@ -1,14 +0,0 @@
-#! /bin/echo must be loaded with .
-
-# Load DPDK devel config and allow override
-# from system file
-test ! -r /etc/dpdk/devel.config ||
-. /etc/dpdk/devel.config
-# from user file
-test ! -r ~/.config/dpdk/devel.config ||
-. ~/.config/dpdk/devel.config
-# from local file
-test ! -r $(dirname $(readlink -m $0))/../.develconfig ||
-. $(dirname $(readlink -m $0))/../.develconfig
-
-# The config files must export variables in the shell style
diff --git a/scripts/test-build.sh b/scripts/test-build.sh
index d2cafc1..e8971fe 100755
--- a/scripts/test-build.sh
+++ b/scripts/test-build.sh
@@ -48,7 +48,7 @@ default_path=$PATH
 # - DPDK_NOTIFY (notify-send)
 # - LIBSSO_SNOW3G_PATH
 # - LIBSSO_KASUMI_PATH
-. $(dirname $(readlink -e $0))/load-devel-config.sh
+. $(dirname $(readlink -e $0))/load-devel-config

 print_usage () {
echo "usage: $(basename $0) [-h] [-jX] [-s] [config1 [config2] ...]]"
@@ -211,7 +211,7 @@ for conf in $configs ; do
# reload config with DPDK_TARGET set
DPDK_TARGET=$target
reset_env
-   . $(dirname $(readlink -e $0))/load-devel-config.sh
+   . $(dirname $(readlink -e $0))/load-devel-config

options=$(echo $conf | sed 's,[^~+]*,,')
dir=$conf
-- 
2.7.4



[dpdk-dev] eal: map shared config into exact same address as primary process (for freebsd)

2016-08-02 Thread txcy uio
Hello

I am getting a segmentation fault on freebsd 10 due to the shared config
mismatch between the primary and secondary process -

The following commit seems to have fixed this issue long time back on Linux
but was never ported to freebsd.
http://dpdk.org/browse/dpdk/commit/?id=6258f1c942c3e0b7bb715158cc266eede6521ddf

Was there any specific reason for not fixing this on freebsd?

FYI -

secondary process :

(gdb) p rte_config
$2 = {master_lcore = 7, lcore_count = 2, lcore_role = {ROLE_OFF, ROLE_OFF,
ROLE_OFF, ROLE_OFF, ROLE_OFF, ROLE_OFF, ROLE_RTE,
ROLE_RTE, ROLE_OFF }, process_type =
RTE_PROC_SECONDARY, mem_config = 0x80067}

primary proces :

(gdb) p rte_config
$2 = {master_lcore = 0, lcore_count = 6, lcore_role = {ROLE_RTE, ROLE_RTE,
ROLE_RTE, ROLE_RTE, ROLE_RTE, ROLE_RTE,
ROLE_OFF }, process_type = RTE_PROC_PRIMARY,
mem_config = 0x800675000}

--tyc


[dpdk-dev] how to design high performance QoS support for a large amount of subscribers

2016-08-02 Thread Yuyong Zhang
Hi,

I am trying to add QoS support for a high performance VNF with large amount of 
subscribers (millions). It requires to support guaranteed bit rate for 
different service level of subscribers. I.e. four service levels need to be 
supported:

* Diamond, 500M

* Gold, 100M

* Silver, 50M

* Bronze, 10M

Here is the current pipeline design using DPDK:


* 4 RX threads, does packet classification and load balancing

* 10-20 worker thread, does application subscriber management

* 4 TX threads, sends packets to TX NICs.

* Ring buffers used among RX threads, Worker threads, and TX threads

I read DPDK program guide for QoS framework regarding  hierarchical scheduler: 
Port, sub-port, pipe, TC and queues, I am looking for advice on how to design 
QoS scheduler to support millions of subscribers (pipes) which traffic are 
processed in tens of worker threads where subscriber management processing are 
handled?

One design thought is as the following:

8 ports (each one is associated with one physical port), 16-20 sub-ports (each 
is used by one Worker thread), each sub-port supports 250K pipes for 
subscribers. Each worker thread manages one sub-port and does metering for the 
sub-port to get color, and after identity subscriber flow pick a unused pipe, 
and do sched enqueuer/de-queue and then put into TX rings to TX threads, and TX 
threads send the packets to TX NICs.

Are there functional and performance issues with above approach?

Any advice and input are appreciated.

Regards,

Yuyong






[dpdk-dev] [PATCH v2] net/mlx5: Fix possible NULL deref in RX path

2016-08-02 Thread Sagi Grimberg
The user is allowed to call ->rx_pkt_burst() even without free
mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
on the first iteration (where pkt is still NULL). This would cause us
to deref a NULL pkt (reset refcount and free).

Fix this by checking the pkt before freeing it.

Fixes: a1bdb71a32da ("net/mlx5: fix crash in Rx")
Signed-off-by: Sagi Grimberg 
---
Changes from v1:
- check pkt only once in case we failed to allocate a buffer

 drivers/net/mlx5/mlx5_rxtx.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index fce3381ae87a..37573668e43e 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, 
uint16_t pkts_n)
rte_prefetch0(wqe);
rep = rte_mbuf_raw_alloc(rxq->mp);
if (unlikely(rep == NULL)) {
+   ++rxq->stats.rx_nombuf;
+   if (!pkt) {
+   /*
+* no buffers before we even started,
+* bail out silently.
+*/
+   break;
+   }
while (pkt != seg) {
assert(pkt != (*rxq->elts)[idx]);
seg = NEXT(pkt);
@@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, 
uint16_t pkts_n)
__rte_mbuf_raw_free(pkt);
pkt = seg;
}
-   ++rxq->stats.rx_nombuf;
break;
}
if (!pkt) {
-- 
1.9.1



[dpdk-dev] DPDK Community Survey - about to close

2016-08-02 Thread Thomas Monjalon
Just 2 minutes for DPDK ;)
http://surveymonkey.com/r/DPDK_Community_Survey


2016-08-02 14:39, Thomas Monjalon:
> Hi all,
> 
> That's the first time a DPDK survey is published.
> It will help us in our future progress to decide what are the most
> important stuff to work on.
> 
> Please do not wait to fill it out. It closes on August 4.
> Thanks for taking 2 minutes now to give your feedback.
> When you will have done your duty, you might peacefully do
> your next task or just enjoy a nice August month :)
> 
> Each voice counts! Thanks
> 
> 
> 2016-07-28 16:45, Glynn, Michael J:
> > Hi all
> > 
> > As part of our ongoing efforts to improve DPDK, we'd like to hear
> > your feedback!
> > 
> > We have created a number of DPDK-related questions here
> > https://www.surveymonkey.com/r/DPDK_Community_Survey
> > and want to hear your views!!
> > 
> > The survey will close at midnight GMT on Thursday August 4th
> > 
> > Thanks in advance for your feedback - the more responses we get
> > the more data we have to drive further features, improvements, etc...
> > so please respond!!
> > 
> > Regards
> > Mike
> 




[dpdk-dev] [dpdk-announce] DPDK Community Survey - about to close

2016-08-02 Thread Thomas Monjalon
Hi all,

That's the first time a DPDK survey is published.
It will help us in our future progress to decide what are the most
important stuff to work on.

Please do not wait to fill it out. It closes on August 4.
Thanks for taking 2 minutes now to give your feedback.
When you will have done your duty, you might peacefully do
your next task or just enjoy a nice August month :)

Each voice counts! Thanks


2016-07-28 16:45, Glynn, Michael J:
> Hi all
> 
> As part of our ongoing efforts to improve DPDK, we'd like to hear
> your feedback!
> 
> We have created a number of DPDK-related questions here
> https://www.surveymonkey.com/r/DPDK_Community_Survey
> and want to hear your views!!
> 
> The survey will close at midnight GMT on Thursday August 4th
> 
> Thanks in advance for your feedback - the more responses we get
> the more data we have to drive further features, improvements, etc...
> so please respond!!
> 
> Regards
> Mike



[dpdk-dev] [PATCH v2] net/mlx5: Fix possible NULL deref in RX path

2016-08-02 Thread Adrien Mazarguil
On Tue, Aug 02, 2016 at 03:02:18PM +0300, Sagi Grimberg wrote:
> The user is allowed to call ->rx_pkt_burst() even without free
> mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
> on the first iteration (where pkt is still NULL). This would cause us
> to deref a NULL pkt (reset refcount and free).
> 
> Fix this by checking the pkt before freeing it.
> 
> Fixes: a1bdb71a32da ("net/mlx5: fix crash in Rx")
> Signed-off-by: Sagi Grimberg 
> ---
> Changes from v1:
> - check pkt only once in case we failed to allocate a buffer
> 
>  drivers/net/mlx5/mlx5_rxtx.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index fce3381ae87a..37573668e43e 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, 
> uint16_t pkts_n)
>   rte_prefetch0(wqe);
>   rep = rte_mbuf_raw_alloc(rxq->mp);
>   if (unlikely(rep == NULL)) {
> + ++rxq->stats.rx_nombuf;
> + if (!pkt) {
> + /*
> +  * no buffers before we even started,
> +  * bail out silently.
> +  */
> + break;
> + }
>   while (pkt != seg) {
>   assert(pkt != (*rxq->elts)[idx]);
>   seg = NEXT(pkt);
> @@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, 
> uint16_t pkts_n)
>   __rte_mbuf_raw_free(pkt);
>   pkt = seg;
>   }
> - ++rxq->stats.rx_nombuf;
>   break;
>   }
>   if (!pkt) {
> -- 
> 1.9.1

A few nit-picks from check-git-log.sh:

 Wrong headline uppercase:
 net/mlx5: Fix possible NULL deref in RX path
 Wrong headline lowercase:
 net/mlx5: Fix possible NULL deref in RX path
 Missing blank line after 'Fixes' tag:
 Fixes: a1bdb71a32da ("net/mlx5: fix crash in Rx")

Otherwise,

Acked-by: Adrien Mazarguil 

-- 
Adrien Mazarguil
6WIND


[dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path

2016-08-02 Thread Sagi Grimberg


On 02/08/16 12:58, Adrien Mazarguil wrote:
> On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote:
>>
>>
>> On 01/08/16 19:43, Adrien Mazarguil wrote:
>>> Hi Sagi,
>>>
>>> On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote:
 The user is allowed to call ->rx_pkt_burst() even without free
 mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
 on the first iteration (where pkt is still NULL). This would cause us
 to deref a NULL pkt (reset refcount and free).

 Fix this by checking the pkt before freeing it.
>>>
>>> Just to be sure, did you get an actual NULL deref crash here or is that an
>>> assumed possibility?
>>>
>>> I'm asking because this problem was supposed to be addressed by:
>>>
>>> a1bdb71a32da ("net/mlx5: fix crash in Rx")
>>
>> I actually got the NULL deref. This happens when the application doesn't
>> restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
>> will fail on the first iteration (pkt wasn't assigned) unlike the
>> condition handled in a1bdb71a32da.
>>
>> With this applied, I didn't see the crash.
>
> Thanks for confirming this,

Hey Adrien, I just noticed that I missed the rest of
your response in the previous message (pre-coffee mail
browsing...)

You analysis was on spot.

> now what about the different approach I
> suggested in my previous message to avoid the extra check in the inner loop:
>
>  if (!pkt)
>  pkt = seg;
>  while (pkt != seg) {
>   ...
>  }

We can go this way, but it looks kinda confusing to set pkt = seg and
then iterate on pkt != seg.

How about a more explicit approach:
--
diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index fce3381ae87a..37573668e43e 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf 
**pkts, uint16_t pkts_n)
 rte_prefetch0(wqe);
 rep = rte_mbuf_raw_alloc(rxq->mp);
 if (unlikely(rep == NULL)) {
+   ++rxq->stats.rx_nombuf;
+   if (!pkt) {
+   /*
+* no buffers before we even started,
+* bail out silently.
+*/
+   break;
+   }
 while (pkt != seg) {
 assert(pkt != (*rxq->elts)[idx]);
 seg = NEXT(pkt);
@@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf 
**pkts, uint16_t pkts_n)
 __rte_mbuf_raw_free(pkt);
 pkt = seg;
 }
-   ++rxq->stats.rx_nombuf;
 break;
 }
 if (!pkt) {
--


> Also the fixes line in your commit message?

I'll add it in v2. Thanks.



[dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path

2016-08-02 Thread Adrien Mazarguil
On Tue, Aug 02, 2016 at 01:47:55PM +0300, Sagi Grimberg wrote:
> 
> 
> On 02/08/16 12:58, Adrien Mazarguil wrote:
> >On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote:
> >>
> >>
> >>On 01/08/16 19:43, Adrien Mazarguil wrote:
> >>>Hi Sagi,
> >>>
> >>>On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote:
> The user is allowed to call ->rx_pkt_burst() even without free
> mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
> on the first iteration (where pkt is still NULL). This would cause us
> to deref a NULL pkt (reset refcount and free).
> 
> Fix this by checking the pkt before freeing it.
> >>>
> >>>Just to be sure, did you get an actual NULL deref crash here or is that an
> >>>assumed possibility?
> >>>
> >>>I'm asking because this problem was supposed to be addressed by:
> >>>
> >>>a1bdb71a32da ("net/mlx5: fix crash in Rx")
> >>
> >>I actually got the NULL deref. This happens when the application doesn't
> >>restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
> >>will fail on the first iteration (pkt wasn't assigned) unlike the
> >>condition handled in a1bdb71a32da.
> >>
> >>With this applied, I didn't see the crash.
> >
> >Thanks for confirming this,
> 
> Hey Adrien, I just noticed that I missed the rest of
> your response in the previous message (pre-coffee mail
> browsing...)
> 
> You analysis was on spot.
> 
> >now what about the different approach I
> >suggested in my previous message to avoid the extra check in the inner loop:
> >
> > if (!pkt)
> > pkt = seg;
> > while (pkt != seg) {
> >  ...
> > }
> 
> We can go this way, but it looks kinda confusing to set pkt = seg and
> then iterate on pkt != seg.
> 
> How about a more explicit approach:
> --
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index fce3381ae87a..37573668e43e 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -1572,6 +1572,14 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
> uint16_t pkts_n)
> rte_prefetch0(wqe);
> rep = rte_mbuf_raw_alloc(rxq->mp);
> if (unlikely(rep == NULL)) {
> +   ++rxq->stats.rx_nombuf;
> +   if (!pkt) {
> +   /*
> +* no buffers before we even started,
> +* bail out silently.
> +*/
> +   break;
> +   }
> while (pkt != seg) {
> assert(pkt != (*rxq->elts)[idx]);
> seg = NEXT(pkt);
> @@ -1579,7 +1587,6 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts,
> uint16_t pkts_n)
> __rte_mbuf_raw_free(pkt);
> pkt = seg;
> }
> -   ++rxq->stats.rx_nombuf;
> break;
> }
> if (!pkt) {
> --

Yes, that's also fine.

> >Also the fixes line in your commit message?
> 
> I'll add it in v2. Thanks.

Go ahead, thanks!

-- 
Adrien Mazarguil
6WIND


[dpdk-dev] [PATCH v3 4/4] eal: fix end character check in --lcores argument

2016-08-02 Thread Thomas Monjalon
2016-08-02 08:22, Dai, Wei:
> Hi, Thomas, Yigit
> 
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> > 2016-07-28 16:26, Ferruh Yigit:
> > > On 7/27/2016 12:27 PM, Wei Dai wrote:
> > > > With --lcores 'a-b at c-d', eal_parse_cores() fails because
> > > > eal_parse_set() fails due to the next character after lcore set a-b,
> > > > which is '@'and not ',' or '\0'.
> > > > There is also a right check immediately after this incorrect check.
> > > >
> > > > Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> > > >
> > > > Signed-off-by: Wei Dai 
> > >
> > > I am not sure if a-b at c-d syntax should be supported. (a-b)@(c-d) is
> > > supported and already working.
> > 
> > Agreed.
> > 
> > Series applied, except this last patch 4, thanks
> 
> Sorry, I can't find any document to clarify whether a-b at c-d shoule be 
> supported.
> So I design following case to verify all kinds of combination of - and @. 
> I tested my patch with --lcores '0-3 at 12-15, 4-7@(8-11),  (8-11)@4-7, 
> (12-15)@(0-3), 16-19, (20-23) ',

OK. So do you agree we can discard this patch and forget this syntax?


[dpdk-dev] [PATCH] net/i40e: fix null pointer dereferences when using VMDQ+RSS

2016-08-02 Thread Rich Lane
When using VMDQ+RSS, the queue ids used by the application are not
contiguous (see i40e_pf_config_rss). Most of the driver already handled
this, but there were a few cases where it assumed all configured queues
had been setup.

Fixes: 4861cde46116 ("i40e: new poll mode driver")
Fixes: 6b4537128394 ("i40e: free queue memory when closing")
Fixes: 8e109464c022 ("i40e: allow vector Rx and Tx usage")

Signed-off-by: Rich Lane 
---
 drivers/net/i40e/i40e_rxtx.c | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 554d167..0556a4d 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2948,11 +2948,15 @@ i40e_dev_clear_queues(struct rte_eth_dev *dev)
PMD_INIT_FUNC_TRACE();

for (i = 0; i < dev->data->nb_tx_queues; i++) {
+   if (!dev->data->tx_queues[i])
+   continue;
i40e_tx_queue_release_mbufs(dev->data->tx_queues[i]);
i40e_reset_tx_queue(dev->data->tx_queues[i]);
}

for (i = 0; i < dev->data->nb_rx_queues; i++) {
+   if (!dev->data->rx_queues[i])
+   continue;
i40e_rx_queue_release_mbufs(dev->data->rx_queues[i]);
i40e_reset_rx_queue(dev->data->rx_queues[i]);
}
@@ -2966,12 +2970,16 @@ i40e_dev_free_queues(struct rte_eth_dev *dev)
PMD_INIT_FUNC_TRACE();

for (i = 0; i < dev->data->nb_rx_queues; i++) {
+   if (!dev->data->rx_queues[i])
+   continue;
i40e_dev_rx_queue_release(dev->data->rx_queues[i]);
dev->data->rx_queues[i] = NULL;
}
dev->data->nb_rx_queues = 0;

for (i = 0; i < dev->data->nb_tx_queues; i++) {
+   if (!dev->data->tx_queues[i])
+   continue;
i40e_dev_tx_queue_release(dev->data->tx_queues[i]);
dev->data->tx_queues[i] = NULL;
}
@@ -3154,7 +3162,7 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
struct i40e_rx_queue *rxq =
dev->data->rx_queues[i];

-   if (i40e_rxq_vec_setup(rxq)) {
+   if (rxq && i40e_rxq_vec_setup(rxq)) {
ad->rx_vec_allowed = false;
break;
}
@@ -3216,7 +3224,8 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
for (i = 0; i < dev->data->nb_rx_queues; i++) {
struct i40e_rx_queue *rxq = dev->data->rx_queues[i];

-   rxq->rx_using_sse = rx_using_sse;
+   if (rxq)
+   rxq->rx_using_sse = rx_using_sse;
}
}
 }
@@ -3255,7 +3264,7 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
struct i40e_tx_queue *txq =
dev->data->tx_queues[i];

-   if (i40e_txq_vec_setup(txq)) {
+   if (txq && i40e_txq_vec_setup(txq)) {
ad->tx_vec_allowed = false;
break;
}
-- 
1.9.1



[dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path

2016-08-02 Thread Sagi Grimberg


On 01/08/16 19:43, Adrien Mazarguil wrote:
> Hi Sagi,
>
> On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote:
>> The user is allowed to call ->rx_pkt_burst() even without free
>> mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
>> on the first iteration (where pkt is still NULL). This would cause us
>> to deref a NULL pkt (reset refcount and free).
>>
>> Fix this by checking the pkt before freeing it.
>
> Just to be sure, did you get an actual NULL deref crash here or is that an
> assumed possibility?
>
> I'm asking because this problem was supposed to be addressed by:
>
>  a1bdb71a32da ("net/mlx5: fix crash in Rx")

I actually got the NULL deref. This happens when the application doesn't
restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
will fail on the first iteration (pkt wasn't assigned) unlike the
condition handled in a1bdb71a32da.

With this applied, I didn't see the crash.


[dpdk-dev] [PATCH] i40e: enable i40e pmd on ARM platform

2016-08-02 Thread Jianbo Liu
And add read memory barrier to avoid status inconsistency
between two RX descriptors readings.

Signed-off-by: Jianbo Liu 
---
 config/defconfig_arm64-armv8a-linuxapp-gcc | 2 +-
 doc/guides/nics/overview.rst   | 2 +-
 drivers/net/i40e/i40e_rxtx.c   | 2 ++
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/config/defconfig_arm64-armv8a-linuxapp-gcc 
b/config/defconfig_arm64-armv8a-linuxapp-gcc
index 1a17126..08f282b 100644
--- a/config/defconfig_arm64-armv8a-linuxapp-gcc
+++ b/config/defconfig_arm64-armv8a-linuxapp-gcc
@@ -46,6 +46,6 @@ CONFIG_RTE_EAL_IGB_UIO=n

 CONFIG_RTE_LIBRTE_IVSHMEM=n
 CONFIG_RTE_LIBRTE_FM10K_PMD=n
-CONFIG_RTE_LIBRTE_I40E_PMD=n
+CONFIG_RTE_LIBRTE_I40E_INC_VECTOR=n

 CONFIG_RTE_SCHED_VECTOR=n
diff --git a/doc/guides/nics/overview.rst b/doc/guides/nics/overview.rst
index 6abbae6..5175591 100644
--- a/doc/guides/nics/overview.rst
+++ b/doc/guides/nics/overview.rst
@@ -138,7 +138,7 @@ Most of these differences are summarized below.
Linux VFIO Y Y   Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y  
   Y   Y Y
Other kdrv Y Y  
 Y
ARMv7   
 Y Y Y
-   ARMv8  Y Y Y Y  
 Y Y   Y Y
+   ARMv8  Y   Y Y Y Y  
 Y Y   Y Y
Power8 Y Y  
 Y
TILE-Gx 
 Y
x86-32 Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y Y  
 Y   Y Y Y
diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 554d167..4004b8e 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -994,6 +994,8 @@ i40e_rx_scan_hw_ring(struct i40e_rx_queue *rxq)
I40E_RXD_QW1_STATUS_SHIFT;
}

+   rte_rmb();
+
/* Compute how many status bits were set */
for (j = 0, nb_dd = 0; j < I40E_LOOK_AHEAD; j++)
nb_dd += s[j] & (1 << I40E_RX_DESC_STATUS_DD_SHIFT);
-- 
2.4.11



[dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent

2016-08-02 Thread Kyle Larose
Hello Wei,


On Tue, Aug 2, 2016 at 2:59 AM, Zhao1, Wei  wrote:
> Hi, Wujingjing and Kyle Larose
>
>
>
>> -Original Message-
>> From: Zhao1, Wei
>> Sent: Tuesday, August 2, 2016 11:27 AM
>> To: Wu, Jingjing ; Lu, Wenzhuo
>> 
>> Cc: dev at dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent
>>
>> Hi,Wu jingjing and wenzhuo
>>
>> > -Original Message-
>> > From: Zhao1, Wei
>> > Sent: Monday, August 1, 2016 4:58 PM
>> > To: 'Kyle Larose' 
>> > Cc: dev at dpdk.org
>> > Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic
>> > inconsistent
>> >
>> > Hi, Kyle Larose
>> >The core problem is i40e has no statistic of discard bytes, that
>> > means even if when ports are not stopped, the statistic  rx_good_bytes
>> > is consist of discard
>> > bytes?is that reasonable? In other words, I can just minus discard
>> > bytes from rx_good_bytes if I can get discard bytes number, that is much
>> better.
>> >
>> > -Original Message-
>> > From: Kyle Larose [mailto:eomereadig at gmail.com]
>> > Sent: Saturday, July 30, 2016 1:17 AM
>> > To: Zhao1, Wei 
>> > Cc: dev at dpdk.org
>> > Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic
>> > inconsistent
>> >
>> > On Fri, Jul 29, 2016 at 4:50 AM, Wei Zhao1  wrote:
>> > > rx_good_bytes and rx_good_packets statistic is inconsistent when
>> > > port stopped,ipackets statistic is minus the discard packets but
>> > > rx_bytes statistic not.Also,i40e has no statistic of discard bytes,
>> > > so we have to delete discard packets item from rx_good_packets statistic.
>> > >
>> > > Fixes: 9aace75fc82e ("i40e: fix statistics")
>> > >
>> > > Signed-off-by: Wei Zhao1 
>> > > ---
>> > >  drivers/net/i40e/i40e_ethdev.c | 3 +--
>> > >  1 file changed, 1 insertion(+), 2 deletions(-)
>> > >
>> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
>> > > b/drivers/net/i40e/i40e_ethdev.c index 11a5804..553dfd9 100644
>> > > --- a/drivers/net/i40e/i40e_ethdev.c
>> > > +++ b/drivers/net/i40e/i40e_ethdev.c
>> > > @@ -2319,8 +2319,7 @@ i40e_dev_stats_get(struct rte_eth_dev *dev,
>> > > struct rte_eth_stats *stats)
>> > >
>> > > stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
>> > > pf->main_vsi->eth_stats.rx_multicast +
>> > > -   pf->main_vsi->eth_stats.rx_broadcast -
>> > > -   pf->main_vsi->eth_stats.rx_discards;
>> > > +   pf->main_vsi->eth_stats.rx_broadcast;
>> > > stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
>> > > pf->main_vsi->eth_stats.tx_multicast +
>> > > pf->main_vsi->eth_stats.tx_broadcast;
>> > > --
>> > > 2.5.5
>> > >
>> >
>> > Is it not worse to report a received packet when no packet was
>> > actually received by the upper layers under normal operations than to
>> > ensure that packets and  bytes are consistent when an interface is
>> > stopped? It seems like the first case is much more likely to occur than the
>> second.
>> >
>> > Are we just introducing a new issue to fix another?
>> >
>> > How does this behaviour compare to other NICs? Does the ixgbe report
>> > discarded packets in its ipackets? My reading of the driver is that it 
>> > does not.
>> > In fact, it does something interesting to deal with the
>> > problem:
>> >
>> > from:
>> > http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c
>> >
>> > /*
>> > * An errata states that gprc actually counts good + missed packets:
>> > * Workaround to set gprc to summated queue packet receives */
>> > hw_stats-
>> > >gprc = *total_qprc;
>> >
>> > total_gprc is equal to the sum of the qprc per queue. Can we do
>> > something similar on the i40e instead of adding unicast, mulitcast and
>> broadcast?
>>
>>
>> I have checked ixgbe code about  Rx statistic, in function
>> ixgbe_read_stats_registers() we can find the rx_good_bytes and
>> rx_good_packets statistic.
>> It is listed below, we  can see rx_good_packets is also just addition of 
>> Queue
>> Packets Received Count and  not minused  discard packet number.
>> Is there some wrong of understanding?



My understanding of the problem can be broken into three parts:
 1) In Unicast/Multicast/Broadcast packet counters are counting
packets which were discarded
 2) The corresponding byte counters count packets which were discarded.
 3) There are no discarded byte counters.

Our in bytes counter consists of the sum of in unicast, in multicast,
and in broadcast. This sum includes discarded bytes, which we do not
want, for two reasons. First, it would lead to misleading bitrate
reports: people expect to see the amount of traffic actually handled.
Second, it conflicts with the current packet counters (the counters
without your change).

Obviously if we could count the discarded bytes, we could subtract
them. Alternatively, if we could count only received bytes which were
not discarded, then we would not need to subtract discarded bytes from
the re

[dpdk-dev] [PATCH] net/mlx5: Fix possible NULL deref in RX path

2016-08-02 Thread Adrien Mazarguil
On Tue, Aug 02, 2016 at 12:31:35PM +0300, Sagi Grimberg wrote:
> 
> 
> On 01/08/16 19:43, Adrien Mazarguil wrote:
> >Hi Sagi,
> >
> >On Mon, Aug 01, 2016 at 11:44:21AM +0300, Sagi Grimberg wrote:
> >>The user is allowed to call ->rx_pkt_burst() even without free
> >>mbufs in the pool. In this scenario we'll fail allocating a rep mbuf
> >>on the first iteration (where pkt is still NULL). This would cause us
> >>to deref a NULL pkt (reset refcount and free).
> >>
> >>Fix this by checking the pkt before freeing it.
> >
> >Just to be sure, did you get an actual NULL deref crash here or is that an
> >assumed possibility?
> >
> >I'm asking because this problem was supposed to be addressed by:
> >
> > a1bdb71a32da ("net/mlx5: fix crash in Rx")
> 
> I actually got the NULL deref. This happens when the application doesn't
> restore mbufs to the pool correctly. In the case rte_mbuf_raw_alloc
> will fail on the first iteration (pkt wasn't assigned) unlike the
> condition handled in a1bdb71a32da.
> 
> With this applied, I didn't see the crash.

Thanks for confirming this, now what about the different approach I
suggested in my previous message to avoid the extra check in the inner loop:

 if (!pkt)
 pkt = seg;
 while (pkt != seg) {
  ...
 }

Also the fixes line in your commit message?

-- 
Adrien Mazarguil
6WIND


[dpdk-dev] [PATCH] table: add missing exports

2016-08-02 Thread Dumitrescu, Cristian


> -Original Message-
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Aleksey Katargin
> Sent: Monday, August 1, 2016 10:01 AM
> To: dev at dpdk.org
> Subject: [dpdk-dev] [PATCH] table: add missing exports
> 
> Signed-off-by: Aleksey Katargin 
> ---
>  lib/librte_table/rte_table_version.map | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_table/rte_table_version.map
> b/lib/librte_table/rte_table_version.map
> index 2138698..459c2da 100644
> --- a/lib/librte_table/rte_table_version.map
> +++ b/lib/librte_table/rte_table_version.map
> @@ -3,6 +3,7 @@ DPDK_2.0 {
> 

Acked-by: Cristian Dumitrescu 


Hi Aleksey,

I checked all DPDK releases 2.0 -> 16.07 and yes, you're exactly right. Ouch!!!

Thanks very much for fixing this!

Regards,
Cristian



[dpdk-dev] [RFC] Generic flow director/filtering/classification API

2016-08-02 Thread John Fastabend
On 16-07-23 02:10 PM, John Fastabend wrote:
> On 16-07-21 12:20 PM, Adrien Mazarguil wrote:
>> Hi Jerin,
>>
>> Sorry, looks like I missed your reply. Please see below.
>>
> 
> Hi Adrian,
> 
> Sorry for a bit delay but a few comments that may be worth considering.
> 
> To start with completely agree on the general problem statement and the
> nice summary of all the current models. Also good start on this.
> 
>>
>> Considering that allowed pattern/actions combinations cannot be known in
>> advance and would result in an unpractically large number of capabilities to
>> expose, a method is provided to validate a given rule from the current
>> device configuration state without actually adding it (akin to a "dry run"
>> mode).
> 
> Rather than have a query/validate process why did we jump over having an
> intermediate representation of the capabilities? Here you state it is
> unpractical but we know how to represent parse graphs and the drivers
> could report their supported parse graph via a single query to a middle
> layer.
> 
> This will actually reduce the msg chatter imagine many applications at
> init time or in boundary cases where a large set of applications come
> online at once and start banging on the interface all at once seems less
> than ideal.
> 

A bit more details on possible interface for capabilities query,

One way I've used to describe these graphs from driver to software
stacks is to use a set of structures to build the graph. For fixed
graphs this could just be *.h file for programmable hardware (typically
coming from fw update on nics) the driver can read the parser details
out of firmware and render the structures.

I've done this two ways: one is to define all the fields in their
own structures using something like,

struct field {
char *name;
u32 uid;
u32 bitwidth;
};

This gives a unique id (uid) for each field along with its
width and a user friendly name. The fields are organized into
headers via a header structure,

struct header_node {
char *name;
u32 uid;
u32 *fields;
struct parse_graph *jump;
};

Each node has a unique id and then a list of fields. Where 'fields'
is a list of uid's of fields its also easy enough to embed the field
struct in the header_node if that is simpler its really a style
question.

The 'struct parse_graph' gives the list of edges from this header node
to other header nodes. Using a parse graph structure defined

struct parse_graph {
struct field_reference ref;
__u32 jump_uid;
};

Again as a matter of style you can embed the parse graph in the header
node as I did above or do it as its own object.

The field_reference noted below gives the id of the field and the value
e.g. the tuple (ipv4.protocol, 6) then jump_uid would be the uid of TCP.

struct field_reference {
__u32 header_uid;
__u32 field_uid;
__u32 mask_type;
__u32 type;
__u8  *value;
__u8  *mask;
};

The cost doing all this is some additional overhead at init time. But
building generic function over this and having a set of predefined
uids for well-known protocols such ip, udp, tcp, etc helps. What you
get for the cost is a few things that I think are worth it. (i) Now
new protocols can be added/removed without recompiling DPDK (ii) a
software package can use the capability query to verify the required
protocols are off-loadable vs a possibly large set of test queries and
(iii) when we do the programming of the device we can provide a tuple
(table-uid, header-uid, field-uid, value, mask, priority) and the
middle layer "knowing" the above graph can verify the command so
drivers only ever see "good"  commands, (iv) finally it should be
faster in terms of cmds per second because the drivers can map the
tuple (table, header, field, priority) to a slot efficiently vs
parsing.

IMO point (iii) and (iv) will in practice make the code much simpler
because we can maintain common middle layer and not require parsing
by drivers. Making each driver simpler by abstracting into common
layer.

> Worse in my opinion it requires all drivers to write mostly duplicating
> validation code where a common layer could easily do this if every
> driver reported a common data structure representing its parse graph
> instead. The nice fallout of this initial effort upfront is the driver
> no longer needs to do error handling/checking/etc and can assume all
> rules are correct and valid. It makes driver code much simpler to
> support. And IMO at least by doing this we get some other nice benefits
> described below.
> 
> Another related question is about performance.
> 
>> Creation
>> 
>>
>> Creating a flow rule is similar to validating one, except the rule is
>> actually created.
>>
>> ::
>>
>>  struct rte_flow *
>>  rte_flow_create(uint8_t port_id,
>>  const struct rte_flow_pattern *pattern,
>>  const struct rte_flow_actions *actions);
> 
> I gather this implies that

[dpdk-dev] [PATCH v2] lpm: remove redundant check when adding lpm rule

2016-08-02 Thread Wei Dai
When a rule with depth > 24 is added into an existing
rule with depth <=24, a new tbl8 is allocated, the existing
rule first fulfill whole new tbl8, so the filed vaild of
each entry in this tbl8 is always true and depth of each
entry is always < 24 before adding new rule with depth > 24.

Signed-off-by: Wei Dai 
---
 lib/librte_lpm/rte_lpm.c | 22 ++
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/lib/librte_lpm/rte_lpm.c b/lib/librte_lpm/rte_lpm.c
index 24fec4b..ec67765 100644
--- a/lib/librte_lpm/rte_lpm.c
+++ b/lib/librte_lpm/rte_lpm.c
@@ -931,32 +931,27 @@ add_depth_big_v20(struct rte_lpm_v20 *lpm, uint32_t 
ip_masked, uint8_t depth,
/* Populate new tbl8 with tbl24 value. */
for (i = tbl8_group_start; i < tbl8_group_end; i++) {
lpm->tbl8[i].valid = VALID;
lpm->tbl8[i].depth = lpm->tbl24[tbl24_index].depth;
lpm->tbl8[i].next_hop =
lpm->tbl24[tbl24_index].next_hop;
}

tbl8_index = tbl8_group_start + (ip_masked & 0xFF);

/* Insert new rule into the tbl8 entry. */
for (i = tbl8_index; i < tbl8_index + tbl8_range; i++) {
-   if (!lpm->tbl8[i].valid ||
-   lpm->tbl8[i].depth <= depth) {
-   lpm->tbl8[i].valid = VALID;
-   lpm->tbl8[i].depth = depth;
-   lpm->tbl8[i].next_hop = next_hop;
-
-   continue;
-   }
+   lpm->tbl8[i].valid = VALID;
+   lpm->tbl8[i].depth = depth;
+   lpm->tbl8[i].next_hop = next_hop;
}

/*
 * Update tbl24 entry to point to new tbl8 entry. Note: The
 * ext_flag and tbl8_index need to be updated simultaneously,
 * so assign whole structure in one go.
 */

struct rte_lpm_tbl_entry_v20 new_tbl24_entry = {
{ .group_idx = (uint8_t)tbl8_group_index, },
.valid = VALID,
.valid_group = 1,
@@ -1062,32 +1057,27 @@ add_depth_big_v1604(struct rte_lpm *lpm, uint32_t 
ip_masked, uint8_t depth,
/* Populate new tbl8 with tbl24 value. */
for (i = tbl8_group_start; i < tbl8_group_end; i++) {
lpm->tbl8[i].valid = VALID;
lpm->tbl8[i].depth = lpm->tbl24[tbl24_index].depth;
lpm->tbl8[i].next_hop =
lpm->tbl24[tbl24_index].next_hop;
}

tbl8_index = tbl8_group_start + (ip_masked & 0xFF);

/* Insert new rule into the tbl8 entry. */
for (i = tbl8_index; i < tbl8_index + tbl8_range; i++) {
-   if (!lpm->tbl8[i].valid ||
-   lpm->tbl8[i].depth <= depth) {
-   lpm->tbl8[i].valid = VALID;
-   lpm->tbl8[i].depth = depth;
-   lpm->tbl8[i].next_hop = next_hop;
-
-   continue;
-   }
+   lpm->tbl8[i].valid = VALID;
+   lpm->tbl8[i].depth = depth;
+   lpm->tbl8[i].next_hop = next_hop;
}

/*
 * Update tbl24 entry to point to new tbl8 entry. Note: The
 * ext_flag and tbl8_index need to be updated simultaneously,
 * so assign whole structure in one go.
 */

struct rte_lpm_tbl_entry new_tbl24_entry = {
.group_idx = (uint8_t)tbl8_group_index,
.valid = VALID,
.valid_group = 1,
-- 
2.5.5



[dpdk-dev] [PATCH v2] examples: fix unusual-interpreter

2016-08-02 Thread Dumitrescu, Cristian


> -Original Message-
> From: Christian Ehrhardt [mailto:christian.ehrhardt at canonical.com]
> Sent: Tuesday, August 2, 2016 7:40 AM
> To: christian.ehrhardt at canonical.com; thomas.monjalon at 6wind.com;
> Dumitrescu, Cristian ; dev at dpdk.org
> Subject: [PATCH v2] examples: fix unusual-interpreter
> 
> *update in v2*
> - use #!/usr/bin/env python as usually recommended and suggested in the
>   discussion
> 
> Due to regular lintian checks in Debian packaging it surfaced that these
> two scripts had a space in their #! statement which renders it to be
> human, but not shell readable.
> 
> Fixes: 8673a3e8 ("examples/ip_pipeline: add config diagram generator")
> Fixes: fa667b46 ("examples/ip_pipeline: add core mappings script")
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  examples/ip_pipeline/config/diagram-generator.py| 2 +-
>  examples/ip_pipeline/config/pipeline-to-core-mapping.py | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 

Acked-by: Cristian Dumitrescu 



[dpdk-dev] [PATCH] qat: change the session structure to be variable sized

2016-08-02 Thread John Griffin
This patch changes the qat session data structure sent to qat from a 
fixed size to a variable size which is dependent on the size of 
the chosen algorithm.  
This reduces the amount of bytes which are transferred across
PCIe and thus helps to increase qat performance when the 
accelerator is bound by PCIe.

Signed-off-by: John Griffin 
---
 drivers/crypto/qat/qat_adf/qat_algs.h|   5 +-
 drivers/crypto/qat/qat_adf/qat_algs_build_desc.c | 462 +--
 drivers/crypto/qat/qat_crypto.c  |  15 +-
 3 files changed, 183 insertions(+), 299 deletions(-)

diff --git a/drivers/crypto/qat/qat_adf/qat_algs.h 
b/drivers/crypto/qat/qat_adf/qat_algs.h
index 243c1b4..6a86053 100644
--- a/drivers/crypto/qat/qat_adf/qat_algs.h
+++ b/drivers/crypto/qat/qat_adf/qat_algs.h
@@ -87,8 +87,10 @@ struct qat_session {
enum icp_qat_hw_cipher_mode qat_mode;
enum icp_qat_hw_auth_algo qat_hash_alg;
struct qat_alg_cd cd;
+   uint8_t *cd_cur_ptr;
phys_addr_t cd_paddr;
struct icp_qat_fw_la_bulk_req fw_req;
+   uint8_t aad_len;
struct qat_crypto_instance *inst;
uint8_t salt[ICP_QAT_HW_AES_BLK_SZ];
rte_spinlock_t lock;/* protects this struct */
@@ -115,7 +117,8 @@ int qat_alg_aead_session_create_content_desc_auth(struct 
qat_session *cdesc,
uint32_t digestsize,
unsigned int operation);

-void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr *header);
+void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr *header,
+   uint16_t proto);

 void qat_alg_ablkcipher_init_enc(struct qat_alg_ablkcipher_cd *cd,
int alg, const uint8_t *key,
diff --git a/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c 
b/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
index 185bb33..6de8919 100644
--- a/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
+++ b/drivers/crypto/qat/qat_adf/qat_algs_build_desc.c
@@ -344,7 +344,8 @@ static int qat_alg_do_precomputes(enum icp_qat_hw_auth_algo 
hash_alg,
return 0;
 }

-void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr *header)
+void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr *header,
+   uint16_t proto)
 {
PMD_INIT_FUNC_TRACE();
header->hdr_flags =
@@ -358,7 +359,7 @@ void qat_alg_init_common_hdr(struct icp_qat_fw_comn_req_hdr 
*header)
ICP_QAT_FW_LA_CIPH_IV_FLD_FLAG_SET(header->serv_specif_flags,
   ICP_QAT_FW_CIPH_IV_16BYTE_DATA);
ICP_QAT_FW_LA_PROTO_SET(header->serv_specif_flags,
-   ICP_QAT_FW_LA_NO_PROTO);
+   proto);
ICP_QAT_FW_LA_UPDATE_STATE_SET(header->serv_specif_flags,
   ICP_QAT_FW_LA_NO_UPDATE_STATE);
 }
@@ -375,127 +376,88 @@ int 
qat_alg_aead_session_create_content_desc_cipher(struct qat_session *cdesc,
struct icp_qat_fw_cipher_cd_ctrl_hdr *cipher_cd_ctrl = ptr;
struct icp_qat_fw_auth_cd_ctrl_hdr *hash_cd_ctrl = ptr;
enum icp_qat_hw_cipher_convert key_convert;
+   uint32_t total_key_size;
uint16_t proto = ICP_QAT_FW_LA_NO_PROTO;/* no CCM/GCM/Snow3G */
-   uint16_t cipher_offset = 0;
+   uint16_t cipher_offset, cd_size;

PMD_INIT_FUNC_TRACE();

-   if (cdesc->qat_cmd == ICP_QAT_FW_LA_CMD_HASH_CIPHER &&
-   cdesc->qat_hash_alg != ICP_QAT_HW_AUTH_ALGO_SNOW_3G_UIA2) {
-   cipher =
-   (struct icp_qat_hw_cipher_algo_blk *)((char *)&cdesc->cd +
-   sizeof(struct icp_qat_hw_auth_algo_blk));
-   cipher_offset = sizeof(struct icp_qat_hw_auth_algo_blk);
-   } else {
-   cipher = (struct icp_qat_hw_cipher_algo_blk *)&cdesc->cd;
-   cipher_offset = 0;
-   }
-   /* CD setup */
-   if (cdesc->qat_dir == ICP_QAT_HW_CIPHER_ENCRYPT) {
-   ICP_QAT_FW_LA_RET_AUTH_SET(header->serv_specif_flags,
-   ICP_QAT_FW_LA_RET_AUTH_RES);
-   ICP_QAT_FW_LA_CMP_AUTH_SET(header->serv_specif_flags,
-   ICP_QAT_FW_LA_NO_CMP_AUTH_RES);
-   } else {
+   if (cdesc->qat_cmd == ICP_QAT_FW_LA_CMD_CIPHER) {
+   cd_pars->u.s.content_desc_addr = cdesc->cd_paddr;
+   ICP_QAT_FW_COMN_CURR_ID_SET(cipher_cd_ctrl,
+   ICP_QAT_FW_SLICE_CIPHER);
+   ICP_QAT_FW_COMN_NEXT_ID_SET(cipher_cd_ctrl,
+   ICP_QAT_FW_SLICE_DRAM_WR);
ICP_QAT_FW_LA_RET_AUTH_SET(header->serv_specif_flags,
ICP_QAT_FW_LA_NO_RET_AUTH_RES);
ICP_QAT_FW_LA_CMP_AUTH_SET(header->serv_specif_flags,
- 

[dpdk-dev] [PATCH v2] examples: fix unusual-interpreter

2016-08-02 Thread Christian Ehrhardt
*update in v2*
- use #!/usr/bin/env python as usually recommended and suggested in the
  discussion

Due to regular lintian checks in Debian packaging it surfaced that these
two scripts had a space in their #! statement which renders it to be
human, but not shell readable.

Fixes: 8673a3e8 ("examples/ip_pipeline: add config diagram generator")
Fixes: fa667b46 ("examples/ip_pipeline: add core mappings script")

Signed-off-by: Christian Ehrhardt 
---
 examples/ip_pipeline/config/diagram-generator.py| 2 +-
 examples/ip_pipeline/config/pipeline-to-core-mapping.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/examples/ip_pipeline/config/diagram-generator.py 
b/examples/ip_pipeline/config/diagram-generator.py
index f20cbcb..6b7170b 100755
--- a/examples/ip_pipeline/config/diagram-generator.py
+++ b/examples/ip_pipeline/config/diagram-generator.py
@@ -1,4 +1,4 @@
-#! /usr/bin/python2
+#!/usr/bin/env python

 #   BSD LICENSE
 #
diff --git a/examples/ip_pipeline/config/pipeline-to-core-mapping.py 
b/examples/ip_pipeline/config/pipeline-to-core-mapping.py
index 37b131c..c2050b8 100755
--- a/examples/ip_pipeline/config/pipeline-to-core-mapping.py
+++ b/examples/ip_pipeline/config/pipeline-to-core-mapping.py
@@ -1,4 +1,4 @@
-#! /usr/bin/python2
+#!/usr/bin/env python

 #   BSD LICENSE
 #
-- 
2.7.4



[dpdk-dev] [PATCH v3 4/4] eal: fix end character check in --lcores argument

2016-08-02 Thread Dai, Wei
Hi, Thomas, Yigit

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Friday, July 29, 2016 12:06 AM
> To: Dai, Wei 
> Cc: dev at dpdk.org; Yigit, Ferruh 
> Subject: Re: [dpdk-dev] [PATCH v3 4/4] eal: fix end character check in 
> --lcores
> argument
> 
> 2016-07-28 16:26, Ferruh Yigit:
> > On 7/27/2016 12:27 PM, Wei Dai wrote:
> > > With --lcores 'a-b at c-d', eal_parse_cores() fails because
> > > eal_parse_set() fails due to the next character after lcore set a-b,
> > > which is '@'and not ',' or '\0'.
> > > There is also a right check immediately after this incorrect check.
> > >
> > > Fixes: 53e54bf81700 ("eal: new option --lcores for cpu assignment")
> > >
> > > Signed-off-by: Wei Dai 
> >
> > I am not sure if a-b at c-d syntax should be supported. (a-b)@(c-d) is
> > supported and already working.
> 
> Agreed.
> 
> Series applied, except this last patch 4, thanks

Sorry, I can't find any document to clarify whether a-b at c-d shoule be 
supported.
So I design following case to verify all kinds of combination of - and @. 
I tested my patch with --lcores '0-3 at 12-15, 4-7@(8-11),  (8-11)@4-7, 
(12-15)@(0-3), 16-19, (20-23) ',
After calling eal_parse_args(argc, argv),  I observed lcore_config[i].cpuset to 
check cpuset of each lcore.
I added following codes immediately after calling fctret = eal_parse_args(argc, 
argv); in function rte_eal_init( )
in eal.c to check this patch.

{
struct rte_config *p;
const char *rte_role_str[2] = {"ROLE_RTE", "ROLE_OFF"};
p = rte_eal_get_configuration();
for (i=0; i<28; i++) {
printf("lcore %2d status = %s,  runs on cpuset = 
0x%08lX\n", 
i, rte_role_str[p->lcore_role[i]], 
lcore_config[i].cpuset.__bits[0]);
}
}

The result is as follow:
lcore  0 status = ROLE_RTE,  runs on cpuset = 0xF000
lcore  1 status = ROLE_RTE,  runs on cpuset = 0xF000
lcore  2 status = ROLE_RTE,  runs on cpuset = 0xF000
lcore  3 status = ROLE_RTE,  runs on cpuset = 0xF000
lcore  4 status = ROLE_RTE,  runs on cpuset = 0x0F00
lcore  5 status = ROLE_RTE,  runs on cpuset = 0x0F00
lcore  6 status = ROLE_RTE,  runs on cpuset = 0x0F00
lcore  7 status = ROLE_RTE,  runs on cpuset = 0x0F00
lcore  8 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore  9 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 10 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 11 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 12 status = ROLE_RTE,  runs on cpuset = 0x000F
lcore 13 status = ROLE_RTE,  runs on cpuset = 0x000F
lcore 14 status = ROLE_RTE,  runs on cpuset = 0x000F
lcore 15 status = ROLE_RTE,  runs on cpuset = 0x000F
lcore 16 status = ROLE_RTE,  runs on cpuset = 0x0001
lcore 17 status = ROLE_RTE,  runs on cpuset = 0x0002
lcore 18 status = ROLE_RTE,  runs on cpuset = 0x0004
lcore 19 status = ROLE_RTE,  runs on cpuset = 0x0008
lcore 20 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 21 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 22 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 23 status = ROLE_RTE,  runs on cpuset = 0x00F0
lcore 24 status = ROLE_OFF,  runs on cpuset = 0x
lcore 25 status = ROLE_OFF,  runs on cpuset = 0x
lcore 26 status = ROLE_OFF,  runs on cpuset = 0x
lcore 27 status = ROLE_OFF,  runs on cpuset = 0x




[dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent

2016-08-02 Thread Zhao1, Wei
Hi, Wujingjing and Kyle Larose 



> -Original Message-
> From: Zhao1, Wei
> Sent: Tuesday, August 2, 2016 11:27 AM
> To: Wu, Jingjing ; Lu, Wenzhuo
> 
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent
> 
> Hi,Wu jingjing and wenzhuo
> 
> > -Original Message-
> > From: Zhao1, Wei
> > Sent: Monday, August 1, 2016 4:58 PM
> > To: 'Kyle Larose' 
> > Cc: dev at dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic
> > inconsistent
> >
> > Hi, Kyle Larose
> >The core problem is i40e has no statistic of discard bytes, that
> > means even if when ports are not stopped, the statistic  rx_good_bytes
> > is consist of discard
> > bytes?is that reasonable? In other words, I can just minus discard
> > bytes from rx_good_bytes if I can get discard bytes number, that is much
> better.
> >
> > -Original Message-
> > From: Kyle Larose [mailto:eomereadig at gmail.com]
> > Sent: Saturday, July 30, 2016 1:17 AM
> > To: Zhao1, Wei 
> > Cc: dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic
> > inconsistent
> >
> > On Fri, Jul 29, 2016 at 4:50 AM, Wei Zhao1  wrote:
> > > rx_good_bytes and rx_good_packets statistic is inconsistent when
> > > port stopped,ipackets statistic is minus the discard packets but
> > > rx_bytes statistic not.Also,i40e has no statistic of discard bytes,
> > > so we have to delete discard packets item from rx_good_packets statistic.
> > >
> > > Fixes: 9aace75fc82e ("i40e: fix statistics")
> > >
> > > Signed-off-by: Wei Zhao1 
> > > ---
> > >  drivers/net/i40e/i40e_ethdev.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > > b/drivers/net/i40e/i40e_ethdev.c index 11a5804..553dfd9 100644
> > > --- a/drivers/net/i40e/i40e_ethdev.c
> > > +++ b/drivers/net/i40e/i40e_ethdev.c
> > > @@ -2319,8 +2319,7 @@ i40e_dev_stats_get(struct rte_eth_dev *dev,
> > > struct rte_eth_stats *stats)
> > >
> > > stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
> > > pf->main_vsi->eth_stats.rx_multicast +
> > > -   pf->main_vsi->eth_stats.rx_broadcast -
> > > -   pf->main_vsi->eth_stats.rx_discards;
> > > +   pf->main_vsi->eth_stats.rx_broadcast;
> > > stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
> > > pf->main_vsi->eth_stats.tx_multicast +
> > > pf->main_vsi->eth_stats.tx_broadcast;
> > > --
> > > 2.5.5
> > >
> >
> > Is it not worse to report a received packet when no packet was
> > actually received by the upper layers under normal operations than to
> > ensure that packets and  bytes are consistent when an interface is
> > stopped? It seems like the first case is much more likely to occur than the
> second.
> >
> > Are we just introducing a new issue to fix another?
> >
> > How does this behaviour compare to other NICs? Does the ixgbe report
> > discarded packets in its ipackets? My reading of the driver is that it does 
> > not.
> > In fact, it does something interesting to deal with the
> > problem:
> >
> > from:
> > http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c
> >
> > /*
> > * An errata states that gprc actually counts good + missed packets:
> > * Workaround to set gprc to summated queue packet receives */
> > hw_stats-
> > >gprc = *total_qprc;
> >
> > total_gprc is equal to the sum of the qprc per queue. Can we do
> > something similar on the i40e instead of adding unicast, mulitcast and
> broadcast?
> 
> 
> I have checked ixgbe code about  Rx statistic, in function
> ixgbe_read_stats_registers() we can find the rx_good_bytes and
> rx_good_packets statistic.
> It is listed below, we  can see rx_good_packets is also just addition of Queue
> Packets Received Count and  not minused  discard packet number.
> Is there some wrong of understanding?
> 
> for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++) {
> ..
>  *total_qprc += hw_stats->qprc[i];
>   *total_qbrc += hw_stats->qbrc[i];
> ..
> }

   The problem is i40e has no statistic of discard bytes, so it is impossible 
to minus discard bytes from rx_good_bytes . If you think it's not reasonable to 
Delete  rx_discards iterm from rx_good_packets statistic, this patch will be 
superseded. Because I didn't find other way to correct this problem at present.



[dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent

2016-08-02 Thread Zhao1, Wei
Hi,Wu jingjing and wenzhuo

> -Original Message-
> From: Zhao1, Wei
> Sent: Monday, August 1, 2016 4:58 PM
> To: 'Kyle Larose' 
> Cc: dev at dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent
> 
> Hi, Kyle Larose
>The core problem is i40e has no statistic of discard bytes, that means 
> even if
> when ports are not stopped, the statistic  rx_good_bytes is consist of discard
> bytes?is that reasonable? In other words, I can just minus discard bytes
> from rx_good_bytes if I can get discard bytes number, that is much better.
> 
> -Original Message-
> From: Kyle Larose [mailto:eomereadig at gmail.com]
> Sent: Saturday, July 30, 2016 1:17 AM
> To: Zhao1, Wei 
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: fix Rx statistic inconsistent
> 
> On Fri, Jul 29, 2016 at 4:50 AM, Wei Zhao1  wrote:
> > rx_good_bytes and rx_good_packets statistic is inconsistent when port
> > stopped,ipackets statistic is minus the discard packets but rx_bytes
> > statistic not.Also,i40e has no statistic of discard bytes, so we have
> > to delete discard packets item from rx_good_packets statistic.
> >
> > Fixes: 9aace75fc82e ("i40e: fix statistics")
> >
> > Signed-off-by: Wei Zhao1 
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 11a5804..553dfd9 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -2319,8 +2319,7 @@ i40e_dev_stats_get(struct rte_eth_dev *dev,
> > struct rte_eth_stats *stats)
> >
> > stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
> > pf->main_vsi->eth_stats.rx_multicast +
> > -   pf->main_vsi->eth_stats.rx_broadcast -
> > -   pf->main_vsi->eth_stats.rx_discards;
> > +   pf->main_vsi->eth_stats.rx_broadcast;
> > stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
> > pf->main_vsi->eth_stats.tx_multicast +
> > pf->main_vsi->eth_stats.tx_broadcast;
> > --
> > 2.5.5
> >
> 
> Is it not worse to report a received packet when no packet was actually
> received by the upper layers under normal operations than to ensure that
> packets and  bytes are consistent when an interface is stopped? It seems like
> the first case is much more likely to occur than the second.
> 
> Are we just introducing a new issue to fix another?
> 
> How does this behaviour compare to other NICs? Does the ixgbe report
> discarded packets in its ipackets? My reading of the driver is that it does 
> not.
> In fact, it does something interesting to deal with the
> problem:
> 
> from: http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c
> 
> /*
> * An errata states that gprc actually counts good + missed packets:
> * Workaround to set gprc to summated queue packet receives */ hw_stats-
> >gprc = *total_qprc;
> 
> total_gprc is equal to the sum of the qprc per queue. Can we do something
> similar on the i40e instead of adding unicast, mulitcast and broadcast?


I have checked ixgbe code about  Rx statistic, in function 
ixgbe_read_stats_registers() we can find the rx_good_bytes and rx_good_packets 
statistic.
It is listed below, we  can see rx_good_packets is also just addition of Queue 
Packets Received Count and  not minused  discard packet number.
Is there some wrong of understanding?

for (i = 0; i < IXGBE_QUEUE_STAT_COUNTERS; i++)
{
..  
 *total_qprc += hw_stats->qprc[i];
*total_qbrc += hw_stats->qbrc[i];
..
}