[PATCH] log: deprecate history dump

[PATCH] log: deprecate history dump

[Thomas Monjalon]

The log history uses rte_mempool. In order to remove the mempool
dependency in EAL (and improve the build), this feature is deprecated.
The ABI is kept but the behaviour is now voided because it seems this
function was not used. The history can be read from syslog.

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 app/test-pmd/cmdline.c                       |   3 -
 app/test/autotest_test_funcs.py              |   5 --
 app/test/commands.c                          |   4 +-
 app/test/test_logs.c                         |   3 -
 doc/guides/rel_notes/deprecation.rst         |   3 +
 lib/librte_eal/bsdapp/eal/eal_debug.c        |   6 --
 lib/librte_eal/common/eal_common_log.c       | 128 +--------------------------
 lib/librte_eal/common/include/rte_log.h      |   8 ++
 lib/librte_eal/linuxapp/eal/eal_debug.c      |   6 --
 lib/librte_eal/linuxapp/eal/eal_interrupts.c |   1 -
 lib/librte_eal/linuxapp/eal/eal_ivshmem.c    |   1 -
 lib/librte_eal/linuxapp/eal/eal_log.c        |   3 -
 lib/librte_mempool/rte_mempool.c             |   4 -
 13 files changed, 16 insertions(+), 159 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1921612..fd389ac 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -7268,8 +7268,6 @@ static void cmd_dump_parsed(void *parsed_result,
 		rte_dump_physmem_layout(stdout);
 	else if (!strcmp(res->dump, "dump_memzone"))
 		rte_memzone_dump(stdout);
-	else if (!strcmp(res->dump, "dump_log_history"))
-		rte_log_dump_history(stdout);
 	else if (!strcmp(res->dump, "dump_struct_sizes"))
 		dump_struct_sizes();
 	else if (!strcmp(res->dump, "dump_ring"))
@@ -7284,7 +7282,6 @@ cmdline_parse_token_string_t cmd_dump_dump =
 	TOKEN_STRING_INITIALIZER(struct cmd_dump_result, dump,
 		"dump_physmem#"
 		"dump_memzone#"
-		"dump_log_history#"
 		"dump_struct_sizes#"
 		"dump_ring#"
 		"dump_mempool#"
diff --git a/app/test/autotest_test_funcs.py b/app/test/autotest_test_funcs.py
index b60b941..14cffd0 100644
--- a/app/test/autotest_test_funcs.py
+++ b/app/test/autotest_test_funcs.py
@@ -144,16 +144,11 @@ def logs_autotest(child, test_name):
 	i = 0
 	child.sendline(test_name)
 
-	# logs sequence is printed twice because of history dump
 	log_list = [
 		"TESTAPP1: error message",
 		"TESTAPP1: critical message",
 		"TESTAPP2: critical message",
 		"TESTAPP1: error message",
-		"TESTAPP1: error message",
-		"TESTAPP1: critical message",
-		"TESTAPP2: critical message",
-		"TESTAPP1: error message",
 	]
 
 	for log_msg in log_list:
diff --git a/app/test/commands.c b/app/test/commands.c
index e0af8e4..2df46b0 100644
--- a/app/test/commands.c
+++ b/app/test/commands.c
@@ -150,8 +150,6 @@ static void cmd_dump_parsed(void *parsed_result,
 		rte_dump_physmem_layout(stdout);
 	else if (!strcmp(res->dump, "dump_memzone"))
 		rte_memzone_dump(stdout);
-	else if (!strcmp(res->dump, "dump_log_history"))
-		rte_log_dump_history(stdout);
 	else if (!strcmp(res->dump, "dump_struct_sizes"))
 		dump_struct_sizes();
 	else if (!strcmp(res->dump, "dump_ring"))
@@ -164,7 +162,7 @@ static void cmd_dump_parsed(void *parsed_result,
 
 cmdline_parse_token_string_t cmd_dump_dump =
 	TOKEN_STRING_INITIALIZER(struct cmd_dump_result, dump,
-				 "dump_physmem#dump_memzone#dump_log_history#"
+				 "dump_physmem#dump_memzone#"
 				 "dump_struct_sizes#dump_ring#dump_mempool#"
 				 "dump_devargs");
 
diff --git a/app/test/test_logs.c b/app/test/test_logs.c
index 05aa862..d0a9962 100644
--- a/app/test/test_logs.c
+++ b/app/test/test_logs.c
@@ -83,9 +83,6 @@ test_logs(void)
 	RTE_LOG(ERR, TESTAPP1, "error message\n");
 	RTE_LOG(ERR, TESTAPP2, "error message (not displayed)\n");
 
-	/* print again the previous logs */
-	rte_log_dump_history(stdout);
-
 	return 0;
 }
 
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ad05eba..f11df93 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -8,6 +8,9 @@ API and ABI deprecation notices are to be posted here.
 Deprecation Notices
 -------------------
 
+* The log history is deprecated in release 16.07.
+  It is voided and will be completely removed in release 16.11.
+
 * The ethdev hotplug API is going to be moved to EAL with a notification
   mechanism added to crypto and ethdev libraries so that hotplug is now
   available to both of them. This API will be stripped of the device arguments
diff --git a/lib/librte_eal/bsdapp/eal/eal_debug.c b/lib/librte_eal/bsdapp/eal/eal_debug.c
index 907fbfa..5fbc17c 100644
--- a/lib/librte_eal/bsdapp/eal/eal_debug.c
+++ b/lib/librte_eal/bsdapp/eal/eal_debug.c
@@ -77,9 +77,6 @@ void __rte_panic(const char *funcname, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	rte_log(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, "PANIC in %s():\n", funcname);
 	va_start(ap, format);
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
@@ -98,9 +95,6 @@ rte_exit(int exit_code, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	if (exit_code != 0)
 		RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
 				"  Cause: ", exit_code);
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index b5e37bb..94ecdd2 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -56,29 +56,11 @@
 #include <rte_spinlock.h>
 #include <rte_branch_prediction.h>
 #include <rte_ring.h>
-#include <rte_mempool.h>
 
 #include "eal_private.h"
 
 #define LOG_ELT_SIZE     2048
 
-#define LOG_HISTORY_MP_NAME "log_history"
-
-STAILQ_HEAD(log_history_list, log_history);
-
-/**
- * The structure of a message log in the log history.
- */
-struct log_history {
-	STAILQ_ENTRY(log_history) next;
-	unsigned size;
-	char buf[0];
-};
-
-static struct rte_mempool *log_history_mp = NULL;
-static unsigned log_history_size = 0;
-static struct log_history_list log_history;
-
 /* global log structure */
 struct rte_logs rte_logs = {
 	.type = ~0,
@@ -86,10 +68,7 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
-static rte_spinlock_t log_dump_lock = RTE_SPINLOCK_INITIALIZER;
-static rte_spinlock_t log_list_lock = RTE_SPINLOCK_INITIALIZER;
 static FILE *default_log_stream;
-static int history_enabled = 1;
 
 /**
  * This global structure stores some informations about the message
@@ -106,59 +85,14 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 /* default logs */
 
 int
-rte_log_add_in_history(const char *buf, size_t size)
+rte_log_add_in_history(const char *buf __rte_unused, size_t size __rte_unused)
 {
-	struct log_history *hist_buf = NULL;
-	static const unsigned hist_buf_size = LOG_ELT_SIZE - sizeof(*hist_buf);
-	void *obj;
-
-	if (history_enabled == 0)
-		return 0;
-
-	rte_spinlock_lock(&log_list_lock);
-
-	/* get a buffer for adding in history */
-	if (log_history_size > RTE_LOG_HISTORY) {
-		hist_buf = STAILQ_FIRST(&log_history);
-		if (hist_buf) {
-			STAILQ_REMOVE_HEAD(&log_history, next);
-			log_history_size--;
-		}
-	}
-	else {
-		if (rte_mempool_mc_get(log_history_mp, &obj) < 0)
-			obj = NULL;
-		hist_buf = obj;
-	}
-
-	/* no buffer */
-	if (hist_buf == NULL) {
-		rte_spinlock_unlock(&log_list_lock);
-		return -ENOBUFS;
-	}
-
-	/* not enough room for msg, buffer go back in mempool */
-	if (size >= hist_buf_size) {
-		rte_mempool_mp_put(log_history_mp, hist_buf);
-		rte_spinlock_unlock(&log_list_lock);
-		return -ENOBUFS;
-	}
-
-	/* add in history */
-	memcpy(hist_buf->buf, buf, size);
-	hist_buf->buf[size] = hist_buf->buf[hist_buf_size-1] = '\0';
-	hist_buf->size = size;
-	STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
-	log_history_size++;
-	rte_spinlock_unlock(&log_list_lock);
-
 	return 0;
 }
 
 void
-rte_log_set_history(int enable)
+rte_log_set_history(int enable __rte_unused)
 {
-	history_enabled = enable;
 }
 
 /* Change the stream that will be used by logging system */
@@ -217,44 +151,8 @@ int rte_log_cur_msg_logtype(void)
 
 /* Dump log history to file */
 void
-rte_log_dump_history(FILE *out)
+rte_log_dump_history(FILE *out __rte_unused)
 {
-	struct log_history_list tmp_log_history;
-	struct log_history *hist_buf;
-	unsigned i;
-
-	/* only one dump at a time */
-	rte_spinlock_lock(&log_dump_lock);
-
-	/* save list, and re-init to allow logging during dump */
-	rte_spinlock_lock(&log_list_lock);
-	tmp_log_history = log_history;
-	STAILQ_INIT(&log_history);
-	log_history_size = 0;
-	rte_spinlock_unlock(&log_list_lock);
-
-	for (i=0; i<RTE_LOG_HISTORY; i++) {
-
-		/* remove one message from history list */
-		hist_buf = STAILQ_FIRST(&tmp_log_history);
-
-		if (hist_buf == NULL)
-			break;
-
-		STAILQ_REMOVE_HEAD(&tmp_log_history, next);
-
-		/* write on stdout */
-		if (fwrite(hist_buf->buf, hist_buf->size, 1, out) == 0) {
-			rte_mempool_mp_put(log_history_mp, hist_buf);
-			break;
-		}
-
-		/* put back message structure in pool */
-		rte_mempool_mp_put(log_history_mp, hist_buf);
-	}
-	fflush(out);
-
-	rte_spinlock_unlock(&log_dump_lock);
 }
 
 /*
@@ -297,29 +195,11 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function to initialize log
- * history
+ * called by environment-specific log init function
  */
 int
 rte_eal_common_log_init(FILE *default_log)
 {
-	STAILQ_INIT(&log_history);
-
-	/* reserve RTE_LOG_HISTORY*2 elements, so we can dump and
-	 * keep logging during this time */
-	log_history_mp = rte_mempool_create(LOG_HISTORY_MP_NAME, RTE_LOG_HISTORY*2,
-				LOG_ELT_SIZE, 0, 0,
-				NULL, NULL,
-				NULL, NULL,
-				SOCKET_ID_ANY, MEMPOOL_F_NO_PHYS_CONTIG);
-
-	if ((log_history_mp == NULL) &&
-	    ((log_history_mp = rte_mempool_lookup(LOG_HISTORY_MP_NAME)) == NULL)){
-		RTE_LOG(ERR, EAL, "%s(): cannot create log_history mempool\n",
-			__func__);
-		return -1;
-	}
-
 	default_log_stream = default_log;
 	rte_openlog_stream(default_log);
 
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 2e47e7f..b1add04 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -42,6 +42,8 @@
  * This file provides a log API to RTE applications.
  */
 
+#include "rte_common.h" /* for __rte_deprecated macro */
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -179,22 +181,27 @@ int rte_log_cur_msg_loglevel(void);
 int rte_log_cur_msg_logtype(void);
 
 /**
+ * @deprecated
  * Enable or disable the history (enabled by default)
  *
  * @param enable
  *   true to enable, or 0 to disable history.
  */
+__rte_deprecated
 void rte_log_set_history(int enable);
 
 /**
+ * @deprecated
  * Dump the log history to a file
  *
  * @param f
  *   A pointer to a file for output
  */
+__rte_deprecated
 void rte_log_dump_history(FILE *f);
 
 /**
+ * @deprecated
  * Add a log message to the history.
  *
  * This function can be called from a user-defined log stream. It adds
@@ -209,6 +216,7 @@ void rte_log_dump_history(FILE *f);
  *   - 0: Success.
  *   - (-ENOBUFS) if there is no room to store the message.
  */
+__rte_deprecated
 int rte_log_add_in_history(const char *buf, size_t size);
 
 /**
diff --git a/lib/librte_eal/linuxapp/eal/eal_debug.c b/lib/librte_eal/linuxapp/eal/eal_debug.c
index 907fbfa..5fbc17c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_debug.c
+++ b/lib/librte_eal/linuxapp/eal/eal_debug.c
@@ -77,9 +77,6 @@ void __rte_panic(const char *funcname, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	rte_log(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, "PANIC in %s():\n", funcname);
 	va_start(ap, format);
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
@@ -98,9 +95,6 @@ rte_exit(int exit_code, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	if (exit_code != 0)
 		RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
 				"  Cause: ", exit_code);
diff --git a/lib/librte_eal/linuxapp/eal/eal_interrupts.c b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
index 06b26a9..0bee8aa 100644
--- a/lib/librte_eal/linuxapp/eal/eal_interrupts.c
+++ b/lib/librte_eal/linuxapp/eal/eal_interrupts.c
@@ -60,7 +60,6 @@
 #include <rte_ring.h>
 #include <rte_debug.h>
 #include <rte_log.h>
-#include <rte_mempool.h>
 #include <rte_pci.h>
 #include <rte_malloc.h>
 #include <rte_errno.h>
diff --git a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
index eea0314..67b3caf 100644
--- a/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
+++ b/lib/librte_eal/linuxapp/eal/eal_ivshmem.c
@@ -49,7 +49,6 @@
 #include <rte_string_fns.h>
 #include <rte_errno.h>
 #include <rte_ring.h>
-#include <rte_mempool.h>
 #include <rte_malloc.h>
 #include <rte_common.h>
 #include <rte_ivshmem.h>
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index 0b133c3..8464152 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -60,9 +60,6 @@ console_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
 	ssize_t ret;
 	uint32_t loglevel;
 
-	/* add this log in history */
-	rte_log_add_in_history(buf, size);
-
 	/* write on stdout */
 	ret = fwrite(buf, 1, size, stdout);
 	fflush(stdout);
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index b54de43..22a5645 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1003,7 +1003,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 
 		if (free == 0) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE1) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1012,7 +1011,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 			hdr->cookie = RTE_MEMPOOL_HEADER_COOKIE2;
 		} else if (free == 1) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE2) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1022,7 +1020,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 		} else if (free == 2) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE1 &&
 			    cookie != RTE_MEMPOOL_HEADER_COOKIE2) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1032,7 +1029,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 		tlr = __mempool_get_trailer(obj);
 		cookie = tlr->cookie;
 		if (cookie != RTE_MEMPOOL_TRAILER_COOKIE) {
-			rte_log_set_history(0);
 			RTE_LOG(CRIT, MEMPOOL,
 				"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 				obj, (const void *) mp, cookie);
-- 
2.7.0

Re: [PATCH] log: deprecate history dump

[David Marchand]

Thomas,

On Thu, Jun 9, 2016 at 4:09 PM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> The log history uses rte_mempool. In order to remove the mempool
> dependency in EAL (and improve the build), this feature is deprecated.
> The ABI is kept but the behaviour is now voided because it seems this
> function was not used. The history can be read from syslog.

It does look like it is not really used.
I am for this change unless someone complains.

Comments below.

> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
>  app/test-pmd/cmdline.c                       |   3 -
>  app/test/autotest_test_funcs.py              |   5 --
>  app/test/commands.c                          |   4 +-
>  app/test/test_logs.c                         |   3 -
>  doc/guides/rel_notes/deprecation.rst         |   3 +
>  lib/librte_eal/bsdapp/eal/eal_debug.c        |   6 --
>  lib/librte_eal/common/eal_common_log.c       | 128 +--------------------------
>  lib/librte_eal/common/include/rte_log.h      |   8 ++
>  lib/librte_eal/linuxapp/eal/eal_debug.c      |   6 --
>  lib/librte_eal/linuxapp/eal/eal_interrupts.c |   1 -
>  lib/librte_eal/linuxapp/eal/eal_ivshmem.c    |   1 -
>  lib/librte_eal/linuxapp/eal/eal_log.c        |   3 -
>  lib/librte_mempool/rte_mempool.c             |   4 -
>  13 files changed, 16 insertions(+), 159 deletions(-)

- You missed autotest_data.py.

$ git grep dump_log_history
app/test/autotest_data.py:               "Command" :    "dump_log_history",

- eal Makefile still refers to librte_mempool.

- Since you are looking at this, what keeps us from removing the
dependency on librte_ring ?
I would say it was mainly because of mempool.
Maybe ivshmem ?


[snip]

> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index ad05eba..f11df93 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -8,6 +8,9 @@ API and ABI deprecation notices are to be posted here.
>  Deprecation Notices
>  -------------------
>
> +* The log history is deprecated in release 16.07.
> +  It is voided and will be completely removed in release 16.11.
> +

It is voided in 16.07 and will be completely removed in release 16.11.


>  * The ethdev hotplug API is going to be moved to EAL with a notification
>    mechanism added to crypto and ethdev libraries so that hotplug is now
>    available to both of them. This API will be stripped of the device arguments

> diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
> index b5e37bb..94ecdd2 100644
> --- a/lib/librte_eal/common/eal_common_log.c
> +++ b/lib/librte_eal/common/eal_common_log.c
> @@ -56,29 +56,11 @@
>  #include <rte_spinlock.h>
>  #include <rte_branch_prediction.h>
>  #include <rte_ring.h>
> -#include <rte_mempool.h>
>
>  #include "eal_private.h"
>
>  #define LOG_ELT_SIZE     2048

We don't need LOG_ELT_SIZE.

>
> -#define LOG_HISTORY_MP_NAME "log_history"
> -
> -STAILQ_HEAD(log_history_list, log_history);
> -
> -/**
> - * The structure of a message log in the log history.
> - */
> -struct log_history {
> -       STAILQ_ENTRY(log_history) next;
> -       unsigned size;
> -       char buf[0];
> -};
> -
> -static struct rte_mempool *log_history_mp = NULL;
> -static unsigned log_history_size = 0;
> -static struct log_history_list log_history;
> -
>  /* global log structure */
>  struct rte_logs rte_logs = {
>         .type = ~0,
> @@ -86,10 +68,7 @@ struct rte_logs rte_logs = {
>         .file = NULL,
>  };
>
> -static rte_spinlock_t log_dump_lock = RTE_SPINLOCK_INITIALIZER;
> -static rte_spinlock_t log_list_lock = RTE_SPINLOCK_INITIALIZER;
>  static FILE *default_log_stream;
> -static int history_enabled = 1;
>
>  /**
>   * This global structure stores some informations about the message
> @@ -106,59 +85,14 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
>  /* default logs */
>
>  int
> -rte_log_add_in_history(const char *buf, size_t size)
> +rte_log_add_in_history(const char *buf __rte_unused, size_t size __rte_unused)
>  {
> -       struct log_history *hist_buf = NULL;
> -       static const unsigned hist_buf_size = LOG_ELT_SIZE - sizeof(*hist_buf);
> -       void *obj;
> -
> -       if (history_enabled == 0)
> -               return 0;
> -
> -       rte_spinlock_lock(&log_list_lock);
> -
> -       /* get a buffer for adding in history */
> -       if (log_history_size > RTE_LOG_HISTORY) {
> -               hist_buf = STAILQ_FIRST(&log_history);
> -               if (hist_buf) {
> -                       STAILQ_REMOVE_HEAD(&log_history, next);
> -                       log_history_size--;
> -               }
> -       }
> -       else {
> -               if (rte_mempool_mc_get(log_history_mp, &obj) < 0)
> -                       obj = NULL;
> -               hist_buf = obj;
> -       }
> -
> -       /* no buffer */
> -       if (hist_buf == NULL) {
> -               rte_spinlock_unlock(&log_list_lock);
> -               return -ENOBUFS;
> -       }
> -
> -       /* not enough room for msg, buffer go back in mempool */
> -       if (size >= hist_buf_size) {
> -               rte_mempool_mp_put(log_history_mp, hist_buf);
> -               rte_spinlock_unlock(&log_list_lock);
> -               return -ENOBUFS;
> -       }
> -
> -       /* add in history */
> -       memcpy(hist_buf->buf, buf, size);
> -       hist_buf->buf[size] = hist_buf->buf[hist_buf_size-1] = '\0';
> -       hist_buf->size = size;
> -       STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
> -       log_history_size++;
> -       rte_spinlock_unlock(&log_list_lock);
> -
>         return 0;
>  }
>
>  void
> -rte_log_set_history(int enable)
> +rte_log_set_history(int enable __rte_unused)
>  {
> -       history_enabled = enable;
>  }

Maybe a warning here for the people who did not read the deprecation notices ?


-- 
David Marchand

Re: [PATCH] log: deprecate history dump

[Thomas Monjalon]

2016-06-09 16:45, David Marchand:
> On Thu, Jun 9, 2016 at 4:09 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > The log history uses rte_mempool. In order to remove the mempool
> > dependency in EAL (and improve the build), this feature is deprecated.
> > The ABI is kept but the behaviour is now voided because it seems this
> > function was not used. The history can be read from syslog.
> 
> It does look like it is not really used.
> I am for this change unless someone complains.
> 
> Comments below.

All your comments will be addressed in a v2. Thanks

> - Since you are looking at this, what keeps us from removing the
> dependency on librte_ring ?

Please see this first small cleanup:
	http://dpdk.org/ml/archives/dev/2016-June/040798.html

> I would say it was mainly because of mempool.
> Maybe ivshmem ?

Yes CONFIG_RTE_LIBRTE_IVSHMEM brings dependencies to rte_ring and rte_ivshmem.
This "feature" also pollute the memory allocator and makes rework harder.
That's why I would be in favor of removing CONFIG_RTE_LIBRTE_IVSHMEM.

Otherwise, as an alternative proposal, the file
	lib/librte_eal/linuxapp/eal/eal_ivshmem.c
could be moved outside of EAL. Probably that lib/librte_ivshmem/
would be a good place.
The tricky operation would be to remove ivshmem init from eal:

#ifdef RTE_LIBRTE_IVSHMEM
    if (rte_eal_ivshmem_init() < 0)                                                                              
        rte_panic("Cannot init IVSHMEM\n");
#endif

    if (rte_eal_memory_init() < 0)
        rte_panic("Cannot init memory\n");

    /* the directories are locked during eal_hugepage_info_init */
    eal_hugedirs_unlock();

    if (rte_eal_memzone_init() < 0)
        rte_panic("Cannot init memzone\n");

    if (rte_eal_tailqs_init() < 0)
        rte_panic("Cannot init tail queues for objects\n");

#ifdef RTE_LIBRTE_IVSHMEM
    if (rte_eal_ivshmem_obj_init() < 0)
        rte_panic("Cannot init IVSHMEM objects\n");
#endif

Re: [PATCH] log: deprecate history dump

[Christian Ehrhardt]

Hi,
in I totally like it - thanks Thomas for picking that up.

I just wanted to mention that the Makefile still refers to mempool, but
David beat me in time and Detail a lot.

I'll certainly try to follow and help the bit I can.



Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd

On Thu, Jun 9, 2016 at 4:45 PM, David Marchand <david.marchand@6wind.com>
wrote:

> Thomas,
>
> On Thu, Jun 9, 2016 at 4:09 PM, Thomas Monjalon
> <thomas.monjalon@6wind.com> wrote:
> > The log history uses rte_mempool. In order to remove the mempool
> > dependency in EAL (and improve the build), this feature is deprecated.
> > The ABI is kept but the behaviour is now voided because it seems this
> > function was not used. The history can be read from syslog.
>
> It does look like it is not really used.
> I am for this change unless someone complains.
>
> Comments below.
>
> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > ---
> >  app/test-pmd/cmdline.c                       |   3 -
> >  app/test/autotest_test_funcs.py              |   5 --
> >  app/test/commands.c                          |   4 +-
> >  app/test/test_logs.c                         |   3 -
> >  doc/guides/rel_notes/deprecation.rst         |   3 +
> >  lib/librte_eal/bsdapp/eal/eal_debug.c        |   6 --
> >  lib/librte_eal/common/eal_common_log.c       | 128
> +--------------------------
> >  lib/librte_eal/common/include/rte_log.h      |   8 ++
> >  lib/librte_eal/linuxapp/eal/eal_debug.c      |   6 --
> >  lib/librte_eal/linuxapp/eal/eal_interrupts.c |   1 -
> >  lib/librte_eal/linuxapp/eal/eal_ivshmem.c    |   1 -
> >  lib/librte_eal/linuxapp/eal/eal_log.c        |   3 -
> >  lib/librte_mempool/rte_mempool.c             |   4 -
> >  13 files changed, 16 insertions(+), 159 deletions(-)
>
> - You missed autotest_data.py.
>
> $ git grep dump_log_history
> app/test/autotest_data.py:               "Command" :    "dump_log_history",
>
> - eal Makefile still refers to librte_mempool.
>
> - Since you are looking at this, what keeps us from removing the
> dependency on librte_ring ?
> I would say it was mainly because of mempool.
> Maybe ivshmem ?
>
>
> [snip]
>
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> > index ad05eba..f11df93 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -8,6 +8,9 @@ API and ABI deprecation notices are to be posted here.
> >  Deprecation Notices
> >  -------------------
> >
> > +* The log history is deprecated in release 16.07.
> > +  It is voided and will be completely removed in release 16.11.
> > +
>
> It is voided in 16.07 and will be completely removed in release 16.11.
>
>
> >  * The ethdev hotplug API is going to be moved to EAL with a notification
> >    mechanism added to crypto and ethdev libraries so that hotplug is now
> >    available to both of them. This API will be stripped of the device
> arguments
>
> > diff --git a/lib/librte_eal/common/eal_common_log.c
> b/lib/librte_eal/common/eal_common_log.c
> > index b5e37bb..94ecdd2 100644
> > --- a/lib/librte_eal/common/eal_common_log.c
> > +++ b/lib/librte_eal/common/eal_common_log.c
> > @@ -56,29 +56,11 @@
> >  #include <rte_spinlock.h>
> >  #include <rte_branch_prediction.h>
> >  #include <rte_ring.h>
> > -#include <rte_mempool.h>
> >
> >  #include "eal_private.h"
> >
> >  #define LOG_ELT_SIZE     2048
>
> We don't need LOG_ELT_SIZE.
>
> >
> > -#define LOG_HISTORY_MP_NAME "log_history"
> > -
> > -STAILQ_HEAD(log_history_list, log_history);
> > -
> > -/**
> > - * The structure of a message log in the log history.
> > - */
> > -struct log_history {
> > -       STAILQ_ENTRY(log_history) next;
> > -       unsigned size;
> > -       char buf[0];
> > -};
> > -
> > -static struct rte_mempool *log_history_mp = NULL;
> > -static unsigned log_history_size = 0;
> > -static struct log_history_list log_history;
> > -
> >  /* global log structure */
> >  struct rte_logs rte_logs = {
> >         .type = ~0,
> > @@ -86,10 +68,7 @@ struct rte_logs rte_logs = {
> >         .file = NULL,
> >  };
> >
> > -static rte_spinlock_t log_dump_lock = RTE_SPINLOCK_INITIALIZER;
> > -static rte_spinlock_t log_list_lock = RTE_SPINLOCK_INITIALIZER;
> >  static FILE *default_log_stream;
> > -static int history_enabled = 1;
> >
> >  /**
> >   * This global structure stores some informations about the message
> > @@ -106,59 +85,14 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg,
> log_cur_msg);
> >  /* default logs */
> >
> >  int
> > -rte_log_add_in_history(const char *buf, size_t size)
> > +rte_log_add_in_history(const char *buf __rte_unused, size_t size
> __rte_unused)
> >  {
> > -       struct log_history *hist_buf = NULL;
> > -       static const unsigned hist_buf_size = LOG_ELT_SIZE -
> sizeof(*hist_buf);
> > -       void *obj;
> > -
> > -       if (history_enabled == 0)
> > -               return 0;
> > -
> > -       rte_spinlock_lock(&log_list_lock);
> > -
> > -       /* get a buffer for adding in history */
> > -       if (log_history_size > RTE_LOG_HISTORY) {
> > -               hist_buf = STAILQ_FIRST(&log_history);
> > -               if (hist_buf) {
> > -                       STAILQ_REMOVE_HEAD(&log_history, next);
> > -                       log_history_size--;
> > -               }
> > -       }
> > -       else {
> > -               if (rte_mempool_mc_get(log_history_mp, &obj) < 0)
> > -                       obj = NULL;
> > -               hist_buf = obj;
> > -       }
> > -
> > -       /* no buffer */
> > -       if (hist_buf == NULL) {
> > -               rte_spinlock_unlock(&log_list_lock);
> > -               return -ENOBUFS;
> > -       }
> > -
> > -       /* not enough room for msg, buffer go back in mempool */
> > -       if (size >= hist_buf_size) {
> > -               rte_mempool_mp_put(log_history_mp, hist_buf);
> > -               rte_spinlock_unlock(&log_list_lock);
> > -               return -ENOBUFS;
> > -       }
> > -
> > -       /* add in history */
> > -       memcpy(hist_buf->buf, buf, size);
> > -       hist_buf->buf[size] = hist_buf->buf[hist_buf_size-1] = '\0';
> > -       hist_buf->size = size;
> > -       STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
> > -       log_history_size++;
> > -       rte_spinlock_unlock(&log_list_lock);
> > -
> >         return 0;
> >  }
> >
> >  void
> > -rte_log_set_history(int enable)
> > +rte_log_set_history(int enable __rte_unused)
> >  {
> > -       history_enabled = enable;
> >  }
>
> Maybe a warning here for the people who did not read the deprecation
> notices ?
>
>
> --
> David Marchand
>

[PATCH v2] log: deprecate history dump

[Thomas Monjalon]

The log history uses rte_mempool. In order to remove the mempool
dependency in EAL (and improve the build), this feature is deprecated.
The ABI is kept but the behaviour is now voided because it seems this
function was not used. The history can be read from syslog.
When enabling the log history, a warning is logged.

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
v2:
- remove more mempool and log history traces
- add a warning if enabling log history
- move not related mempool includes cleanup in another patch
---
 app/test-pmd/cmdline.c                  |   3 -
 app/test/autotest_data.py               |   6 --
 app/test/autotest_test_funcs.py         |   5 --
 app/test/commands.c                     |   4 +-
 app/test/test_logs.c                    |   3 -
 doc/guides/prog_guide/mempool_lib.rst   |   4 +-
 doc/guides/rel_notes/deprecation.rst    |   3 +
 lib/librte_eal/bsdapp/eal/Makefile      |   1 -
 lib/librte_eal/bsdapp/eal/eal_debug.c   |   6 --
 lib/librte_eal/common/eal_common_log.c  | 143 ++------------------------------
 lib/librte_eal/common/eal_private.h     |   3 -
 lib/librte_eal/common/include/rte_log.h |   8 ++
 lib/librte_eal/linuxapp/eal/Makefile    |   1 -
 lib/librte_eal/linuxapp/eal/eal_debug.c |   6 --
 lib/librte_eal/linuxapp/eal/eal_log.c   |   9 +-
 lib/librte_mempool/rte_mempool.c        |   4 -
 16 files changed, 20 insertions(+), 189 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1921612..fd389ac 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -7268,8 +7268,6 @@ static void cmd_dump_parsed(void *parsed_result,
 		rte_dump_physmem_layout(stdout);
 	else if (!strcmp(res->dump, "dump_memzone"))
 		rte_memzone_dump(stdout);
-	else if (!strcmp(res->dump, "dump_log_history"))
-		rte_log_dump_history(stdout);
 	else if (!strcmp(res->dump, "dump_struct_sizes"))
 		dump_struct_sizes();
 	else if (!strcmp(res->dump, "dump_ring"))
@@ -7284,7 +7282,6 @@ cmdline_parse_token_string_t cmd_dump_dump =
 	TOKEN_STRING_INITIALIZER(struct cmd_dump_result, dump,
 		"dump_physmem#"
 		"dump_memzone#"
-		"dump_log_history#"
 		"dump_struct_sizes#"
 		"dump_ring#"
 		"dump_mempool#"
diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 78d2edd..6c87809 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -94,12 +94,6 @@ parallel_test_group_list = [
 		 "Report" :	None,
 		},
 		{
-		 "Name" :	"Dump log history",
-		 "Command" :	"dump_log_history",
-		 "Func" :	dump_autotest,
-		 "Report" :	None,
-		},
-		{
 		 "Name" :	"Dump rings",
 		 "Command" :	"dump_ring",
 		 "Func" :	dump_autotest,
diff --git a/app/test/autotest_test_funcs.py b/app/test/autotest_test_funcs.py
index b60b941..14cffd0 100644
--- a/app/test/autotest_test_funcs.py
+++ b/app/test/autotest_test_funcs.py
@@ -144,16 +144,11 @@ def logs_autotest(child, test_name):
 	i = 0
 	child.sendline(test_name)
 
-	# logs sequence is printed twice because of history dump
 	log_list = [
 		"TESTAPP1: error message",
 		"TESTAPP1: critical message",
 		"TESTAPP2: critical message",
 		"TESTAPP1: error message",
-		"TESTAPP1: error message",
-		"TESTAPP1: critical message",
-		"TESTAPP2: critical message",
-		"TESTAPP1: error message",
 	]
 
 	for log_msg in log_list:
diff --git a/app/test/commands.c b/app/test/commands.c
index e0af8e4..2df46b0 100644
--- a/app/test/commands.c
+++ b/app/test/commands.c
@@ -150,8 +150,6 @@ static void cmd_dump_parsed(void *parsed_result,
 		rte_dump_physmem_layout(stdout);
 	else if (!strcmp(res->dump, "dump_memzone"))
 		rte_memzone_dump(stdout);
-	else if (!strcmp(res->dump, "dump_log_history"))
-		rte_log_dump_history(stdout);
 	else if (!strcmp(res->dump, "dump_struct_sizes"))
 		dump_struct_sizes();
 	else if (!strcmp(res->dump, "dump_ring"))
@@ -164,7 +162,7 @@ static void cmd_dump_parsed(void *parsed_result,
 
 cmdline_parse_token_string_t cmd_dump_dump =
 	TOKEN_STRING_INITIALIZER(struct cmd_dump_result, dump,
-				 "dump_physmem#dump_memzone#dump_log_history#"
+				 "dump_physmem#dump_memzone#"
 				 "dump_struct_sizes#dump_ring#dump_mempool#"
 				 "dump_devargs");
 
diff --git a/app/test/test_logs.c b/app/test/test_logs.c
index 05aa862..d0a9962 100644
--- a/app/test/test_logs.c
+++ b/app/test/test_logs.c
@@ -83,9 +83,6 @@ test_logs(void)
 	RTE_LOG(ERR, TESTAPP1, "error message\n");
 	RTE_LOG(ERR, TESTAPP2, "error message (not displayed)\n");
 
-	/* print again the previous logs */
-	rte_log_dump_history(stdout);
-
 	return 0;
 }
 
diff --git a/doc/guides/prog_guide/mempool_lib.rst b/doc/guides/prog_guide/mempool_lib.rst
index 5fae79a..c3afc2e 100644
--- a/doc/guides/prog_guide/mempool_lib.rst
+++ b/doc/guides/prog_guide/mempool_lib.rst
@@ -38,9 +38,7 @@ In the DPDK, it is identified by name and uses a ring to store free objects.
 It provides some other optional services such as a per-core object cache and
 an alignment helper to ensure that objects are padded to spread them equally on all DRAM or DDR3 channels.
 
-This library is used by the
-:ref:`Mbuf Library <Mbuf_Library>` and the
-:ref:`Environment Abstraction Layer <Environment_Abstraction_Layer>` (for logging history).
+This library is used by the :ref:`Mbuf Library <Mbuf_Library>`.
 
 Cookies
 -------
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ad05eba..bda40c1 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -8,6 +8,9 @@ API and ABI deprecation notices are to be posted here.
 Deprecation Notices
 -------------------
 
+* The log history is deprecated.
+  It is voided in 16.07 and will be removed in release 16.11.
+
 * The ethdev hotplug API is going to be moved to EAL with a notification
   mechanism added to crypto and ethdev libraries so that hotplug is now
   available to both of them. This API will be stripped of the device arguments
diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index 9054ad6..474651b 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -41,7 +41,6 @@ CFLAGS += -I$(SRCDIR)/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_ring
-CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
 CFLAGS += $(WERROR_FLAGS) -O3
 
 LDLIBS += -lexecinfo
diff --git a/lib/librte_eal/bsdapp/eal/eal_debug.c b/lib/librte_eal/bsdapp/eal/eal_debug.c
index 907fbfa..5fbc17c 100644
--- a/lib/librte_eal/bsdapp/eal/eal_debug.c
+++ b/lib/librte_eal/bsdapp/eal/eal_debug.c
@@ -77,9 +77,6 @@ void __rte_panic(const char *funcname, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	rte_log(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, "PANIC in %s():\n", funcname);
 	va_start(ap, format);
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
@@ -98,9 +95,6 @@ rte_exit(int exit_code, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	if (exit_code != 0)
 		RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
 				"  Cause: ", exit_code);
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index b5e37bb..0739331 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -38,47 +38,14 @@
 #include <sys/types.h>
 #include <stdlib.h>
 #include <unistd.h>
-#include <inttypes.h>
-#include <errno.h>
-#include <sys/queue.h>
 
 #include <rte_log.h>
-#include <rte_memory.h>
-#include <rte_memzone.h>
-#include <rte_launch.h>
 #include <rte_common.h>
-#include <rte_cycles.h>
 #include <rte_eal.h>
 #include <rte_per_lcore.h>
-#include <rte_lcore.h>
-#include <rte_atomic.h>
-#include <rte_debug.h>
-#include <rte_spinlock.h>
-#include <rte_branch_prediction.h>
-#include <rte_ring.h>
-#include <rte_mempool.h>
 
 #include "eal_private.h"
 
-#define LOG_ELT_SIZE     2048
-
-#define LOG_HISTORY_MP_NAME "log_history"
-
-STAILQ_HEAD(log_history_list, log_history);
-
-/**
- * The structure of a message log in the log history.
- */
-struct log_history {
-	STAILQ_ENTRY(log_history) next;
-	unsigned size;
-	char buf[0];
-};
-
-static struct rte_mempool *log_history_mp = NULL;
-static unsigned log_history_size = 0;
-static struct log_history_list log_history;
-
 /* global log structure */
 struct rte_logs rte_logs = {
 	.type = ~0,
@@ -86,10 +53,7 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
-static rte_spinlock_t log_dump_lock = RTE_SPINLOCK_INITIALIZER;
-static rte_spinlock_t log_list_lock = RTE_SPINLOCK_INITIALIZER;
 static FILE *default_log_stream;
-static int history_enabled = 1;
 
 /**
  * This global structure stores some informations about the message
@@ -106,59 +70,16 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 /* default logs */
 
 int
-rte_log_add_in_history(const char *buf, size_t size)
+rte_log_add_in_history(const char *buf __rte_unused, size_t size __rte_unused)
 {
-	struct log_history *hist_buf = NULL;
-	static const unsigned hist_buf_size = LOG_ELT_SIZE - sizeof(*hist_buf);
-	void *obj;
-
-	if (history_enabled == 0)
-		return 0;
-
-	rte_spinlock_lock(&log_list_lock);
-
-	/* get a buffer for adding in history */
-	if (log_history_size > RTE_LOG_HISTORY) {
-		hist_buf = STAILQ_FIRST(&log_history);
-		if (hist_buf) {
-			STAILQ_REMOVE_HEAD(&log_history, next);
-			log_history_size--;
-		}
-	}
-	else {
-		if (rte_mempool_mc_get(log_history_mp, &obj) < 0)
-			obj = NULL;
-		hist_buf = obj;
-	}
-
-	/* no buffer */
-	if (hist_buf == NULL) {
-		rte_spinlock_unlock(&log_list_lock);
-		return -ENOBUFS;
-	}
-
-	/* not enough room for msg, buffer go back in mempool */
-	if (size >= hist_buf_size) {
-		rte_mempool_mp_put(log_history_mp, hist_buf);
-		rte_spinlock_unlock(&log_list_lock);
-		return -ENOBUFS;
-	}
-
-	/* add in history */
-	memcpy(hist_buf->buf, buf, size);
-	hist_buf->buf[size] = hist_buf->buf[hist_buf_size-1] = '\0';
-	hist_buf->size = size;
-	STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
-	log_history_size++;
-	rte_spinlock_unlock(&log_list_lock);
-
 	return 0;
 }
 
 void
 rte_log_set_history(int enable)
 {
-	history_enabled = enable;
+	if (enable)
+		RTE_LOG(WARNING, EAL, "The log history is deprecated.\n");
 }
 
 /* Change the stream that will be used by logging system */
@@ -217,44 +138,8 @@ int rte_log_cur_msg_logtype(void)
 
 /* Dump log history to file */
 void
-rte_log_dump_history(FILE *out)
+rte_log_dump_history(FILE *out __rte_unused)
 {
-	struct log_history_list tmp_log_history;
-	struct log_history *hist_buf;
-	unsigned i;
-
-	/* only one dump at a time */
-	rte_spinlock_lock(&log_dump_lock);
-
-	/* save list, and re-init to allow logging during dump */
-	rte_spinlock_lock(&log_list_lock);
-	tmp_log_history = log_history;
-	STAILQ_INIT(&log_history);
-	log_history_size = 0;
-	rte_spinlock_unlock(&log_list_lock);
-
-	for (i=0; i<RTE_LOG_HISTORY; i++) {
-
-		/* remove one message from history list */
-		hist_buf = STAILQ_FIRST(&tmp_log_history);
-
-		if (hist_buf == NULL)
-			break;
-
-		STAILQ_REMOVE_HEAD(&tmp_log_history, next);
-
-		/* write on stdout */
-		if (fwrite(hist_buf->buf, hist_buf->size, 1, out) == 0) {
-			rte_mempool_mp_put(log_history_mp, hist_buf);
-			break;
-		}
-
-		/* put back message structure in pool */
-		rte_mempool_mp_put(log_history_mp, hist_buf);
-	}
-	fflush(out);
-
-	rte_spinlock_unlock(&log_dump_lock);
 }
 
 /*
@@ -297,29 +182,11 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function to initialize log
- * history
+ * called by environment-specific log init function
  */
 int
 rte_eal_common_log_init(FILE *default_log)
 {
-	STAILQ_INIT(&log_history);
-
-	/* reserve RTE_LOG_HISTORY*2 elements, so we can dump and
-	 * keep logging during this time */
-	log_history_mp = rte_mempool_create(LOG_HISTORY_MP_NAME, RTE_LOG_HISTORY*2,
-				LOG_ELT_SIZE, 0, 0,
-				NULL, NULL,
-				NULL, NULL,
-				SOCKET_ID_ANY, MEMPOOL_F_NO_PHYS_CONTIG);
-
-	if ((log_history_mp == NULL) &&
-	    ((log_history_mp = rte_mempool_lookup(LOG_HISTORY_MP_NAME)) == NULL)){
-		RTE_LOG(ERR, EAL, "%s(): cannot create log_history mempool\n",
-			__func__);
-		return -1;
-	}
-
 	default_log_stream = default_log;
 	rte_openlog_stream(default_log);
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 2342fa1..857dc3e 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -49,9 +49,6 @@ int rte_eal_memzone_init(void);
 /**
  * Common log initialization function (private to eal).
  *
- * Called by environment-specific log initialization function to initialize
- * log history.
- *
  * @param default_log
  *   The default log stream to be used.
  * @return
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 2e47e7f..b1add04 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -42,6 +42,8 @@
  * This file provides a log API to RTE applications.
  */
 
+#include "rte_common.h" /* for __rte_deprecated macro */
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -179,22 +181,27 @@ int rte_log_cur_msg_loglevel(void);
 int rte_log_cur_msg_logtype(void);
 
 /**
+ * @deprecated
  * Enable or disable the history (enabled by default)
  *
  * @param enable
  *   true to enable, or 0 to disable history.
  */
+__rte_deprecated
 void rte_log_set_history(int enable);
 
 /**
+ * @deprecated
  * Dump the log history to a file
  *
  * @param f
  *   A pointer to a file for output
  */
+__rte_deprecated
 void rte_log_dump_history(FILE *f);
 
 /**
+ * @deprecated
  * Add a log message to the history.
  *
  * This function can be called from a user-defined log stream. It adds
@@ -209,6 +216,7 @@ void rte_log_dump_history(FILE *f);
  *   - 0: Success.
  *   - (-ENOBUFS) if there is no room to store the message.
  */
+__rte_deprecated
 int rte_log_add_in_history(const char *buf, size_t size);
 
 /**
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index e109361..a0ea698 100644
--- a/lib/librte_eal/linuxapp/eal/Makefile
+++ b/lib/librte_eal/linuxapp/eal/Makefile
@@ -45,7 +45,6 @@ CFLAGS += -I$(SRCDIR)/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_ring
-CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
 CFLAGS += -I$(RTE_SDK)/lib/librte_ivshmem
 CFLAGS += $(WERROR_FLAGS) -O3
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_debug.c b/lib/librte_eal/linuxapp/eal/eal_debug.c
index 907fbfa..5fbc17c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_debug.c
+++ b/lib/librte_eal/linuxapp/eal/eal_debug.c
@@ -77,9 +77,6 @@ void __rte_panic(const char *funcname, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	rte_log(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, "PANIC in %s():\n", funcname);
 	va_start(ap, format);
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
@@ -98,9 +95,6 @@ rte_exit(int exit_code, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	if (exit_code != 0)
 		RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
 				"  Cause: ", exit_code);
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index 0b133c3..d391100 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -50,8 +50,7 @@
 #include "eal_private.h"
 
 /*
- * default log function, used once mempool (hence log history) is
- * available
+ * default log function
  */
 static ssize_t
 console_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
@@ -60,9 +59,6 @@ console_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
 	ssize_t ret;
 	uint32_t loglevel;
 
-	/* add this log in history */
-	rte_log_add_in_history(buf, size);
-
 	/* write on stdout */
 	ret = fwrite(buf, 1, size, stdout);
 	fflush(stdout);
@@ -110,8 +106,7 @@ rte_eal_log_init(const char *id, int facility)
 /* early logs */
 
 /*
- * early log function, used during boot when mempool (hence log
- * history) is not available
+ * early log function, used before rte_eal_log_init
  */
 static ssize_t
 early_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index b54de43..22a5645 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1003,7 +1003,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 
 		if (free == 0) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE1) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1012,7 +1011,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 			hdr->cookie = RTE_MEMPOOL_HEADER_COOKIE2;
 		} else if (free == 1) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE2) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1022,7 +1020,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 		} else if (free == 2) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE1 &&
 			    cookie != RTE_MEMPOOL_HEADER_COOKIE2) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1032,7 +1029,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 		tlr = __mempool_get_trailer(obj);
 		cookie = tlr->cookie;
 		if (cookie != RTE_MEMPOOL_TRAILER_COOKIE) {
-			rte_log_set_history(0);
 			RTE_LOG(CRIT, MEMPOOL,
 				"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 				obj, (const void *) mp, cookie);
-- 
2.7.0

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Thomas Monjalon]

Looking a bit more into librte_ivshmem, the documentation says we need
a Qemu patch but the URL doesn't exist anymore:
	https://01.org/packet-processing/intel%C2%AE-ovdk
	-> 404 Oops, we couldn't find that page

I've never understood why we should keep this wart and now I'm going
to be upset.
To sum up the situation, eal depends on ivshmem which depends on
ring/mempool which depends... on eal.
The truth is that eal should not depends on librte_ivshmem.
And the option CONFIG_RTE_LIBRTE_IVSHMEM should not exist.

There are 3 parts to distinguish:

1/ The librte_ivshmem API to export some data structures from host.
No real problem here.

2/ The scan of the ivshmem devices in the guest init.
It should be handled as any other PCI device with an appropriate driver.
The scan is done by rte_eal_pci_init.

3/ The automatic mapped allocation of DPDK objects in the guest.
It should not be done in EAL.
An ivshmem driver would be called by rte_eal_dev_init.
It would check where are the shared DPDK structures, as currently done
with the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate allocations.
Thus only the driver would depend on ring and mempool.

The last step of the ivshmem cleanup will be to remove the memory hack
RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM could be
removed.

So this is my proposal:
Someone start working on the above cleanup now, otherwise the whole
rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.
We already talked about the rte_ivshmem design issues several times
and nobody declared using it.


---- original thread for reference ----

2016-06-09 17:01, Thomas Monjalon:
> 2016-06-09 16:45, David Marchand:
> > - Since you are looking at this, what keeps us from removing the
> > dependency on librte_ring ?
> 
> Please see this first small cleanup:
> 	http://dpdk.org/ml/archives/dev/2016-June/040798.html
> 
> > I would say it was mainly because of mempool.
> > Maybe ivshmem ?
> 
> Yes CONFIG_RTE_LIBRTE_IVSHMEM brings dependencies to rte_ring and rte_ivshmem.
> This "feature" also pollute the memory allocator and makes rework harder.
> That's why I would be in favor of removing CONFIG_RTE_LIBRTE_IVSHMEM.
> 
> Otherwise, as an alternative proposal, the file
> 	lib/librte_eal/linuxapp/eal/eal_ivshmem.c
> could be moved outside of EAL. Probably that lib/librte_ivshmem/
> would be a good place.
> The tricky operation would be to remove ivshmem init from eal:
> 
> #ifdef RTE_LIBRTE_IVSHMEM
>     if (rte_eal_ivshmem_init() < 0)                                                                              
>         rte_panic("Cannot init IVSHMEM\n");
> #endif
> 
>     if (rte_eal_memory_init() < 0)
>         rte_panic("Cannot init memory\n");
> 
>     /* the directories are locked during eal_hugepage_info_init */
>     eal_hugedirs_unlock();
> 
>     if (rte_eal_memzone_init() < 0)
>         rte_panic("Cannot init memzone\n");
> 
>     if (rte_eal_tailqs_init() < 0)
>         rte_panic("Cannot init tail queues for objects\n");
> 
> #ifdef RTE_LIBRTE_IVSHMEM
>     if (rte_eal_ivshmem_obj_init() < 0)
>         rte_panic("Cannot init IVSHMEM objects\n");
> #endif

[PATCH v3] log: deprecate history dump

[Thomas Monjalon]

The log history uses rte_mempool. In order to remove the mempool
dependency in EAL (and improve the build), this feature is deprecated.
The ABI is kept but the behaviour is now voided because it seems this
function was not used. The history can be read from syslog.

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
v3:
- keep mempool header path in linuxapp for CONFIG_RTE_LIBRTE_IVSHMEM
v2:
- remove more mempool and log history traces
- add a warning if enabling log history
- move not related mempool includes cleanup in another patch
---
 app/test-pmd/cmdline.c                  |   3 -
 app/test/autotest_data.py               |   6 --
 app/test/autotest_test_funcs.py         |   5 --
 app/test/commands.c                     |   4 +-
 app/test/test_logs.c                    |   3 -
 doc/guides/prog_guide/mempool_lib.rst   |   4 +-
 doc/guides/rel_notes/deprecation.rst    |   3 +
 lib/librte_eal/bsdapp/eal/Makefile      |   1 -
 lib/librte_eal/bsdapp/eal/eal_debug.c   |   6 --
 lib/librte_eal/common/eal_common_log.c  | 148 ++------------------------------
 lib/librte_eal/common/eal_private.h     |   3 -
 lib/librte_eal/common/include/rte_log.h |   8 ++
 lib/librte_eal/linuxapp/eal/eal_debug.c |   6 --
 lib/librte_eal/linuxapp/eal/eal_log.c   |   9 +-
 lib/librte_mempool/rte_mempool.c        |   4 -
 15 files changed, 20 insertions(+), 193 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 1921612..fd389ac 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -7268,8 +7268,6 @@ static void cmd_dump_parsed(void *parsed_result,
 		rte_dump_physmem_layout(stdout);
 	else if (!strcmp(res->dump, "dump_memzone"))
 		rte_memzone_dump(stdout);
-	else if (!strcmp(res->dump, "dump_log_history"))
-		rte_log_dump_history(stdout);
 	else if (!strcmp(res->dump, "dump_struct_sizes"))
 		dump_struct_sizes();
 	else if (!strcmp(res->dump, "dump_ring"))
@@ -7284,7 +7282,6 @@ cmdline_parse_token_string_t cmd_dump_dump =
 	TOKEN_STRING_INITIALIZER(struct cmd_dump_result, dump,
 		"dump_physmem#"
 		"dump_memzone#"
-		"dump_log_history#"
 		"dump_struct_sizes#"
 		"dump_ring#"
 		"dump_mempool#"
diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 78d2edd..6c87809 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -94,12 +94,6 @@ parallel_test_group_list = [
 		 "Report" :	None,
 		},
 		{
-		 "Name" :	"Dump log history",
-		 "Command" :	"dump_log_history",
-		 "Func" :	dump_autotest,
-		 "Report" :	None,
-		},
-		{
 		 "Name" :	"Dump rings",
 		 "Command" :	"dump_ring",
 		 "Func" :	dump_autotest,
diff --git a/app/test/autotest_test_funcs.py b/app/test/autotest_test_funcs.py
index b60b941..14cffd0 100644
--- a/app/test/autotest_test_funcs.py
+++ b/app/test/autotest_test_funcs.py
@@ -144,16 +144,11 @@ def logs_autotest(child, test_name):
 	i = 0
 	child.sendline(test_name)
 
-	# logs sequence is printed twice because of history dump
 	log_list = [
 		"TESTAPP1: error message",
 		"TESTAPP1: critical message",
 		"TESTAPP2: critical message",
 		"TESTAPP1: error message",
-		"TESTAPP1: error message",
-		"TESTAPP1: critical message",
-		"TESTAPP2: critical message",
-		"TESTAPP1: error message",
 	]
 
 	for log_msg in log_list:
diff --git a/app/test/commands.c b/app/test/commands.c
index e0af8e4..2df46b0 100644
--- a/app/test/commands.c
+++ b/app/test/commands.c
@@ -150,8 +150,6 @@ static void cmd_dump_parsed(void *parsed_result,
 		rte_dump_physmem_layout(stdout);
 	else if (!strcmp(res->dump, "dump_memzone"))
 		rte_memzone_dump(stdout);
-	else if (!strcmp(res->dump, "dump_log_history"))
-		rte_log_dump_history(stdout);
 	else if (!strcmp(res->dump, "dump_struct_sizes"))
 		dump_struct_sizes();
 	else if (!strcmp(res->dump, "dump_ring"))
@@ -164,7 +162,7 @@ static void cmd_dump_parsed(void *parsed_result,
 
 cmdline_parse_token_string_t cmd_dump_dump =
 	TOKEN_STRING_INITIALIZER(struct cmd_dump_result, dump,
-				 "dump_physmem#dump_memzone#dump_log_history#"
+				 "dump_physmem#dump_memzone#"
 				 "dump_struct_sizes#dump_ring#dump_mempool#"
 				 "dump_devargs");
 
diff --git a/app/test/test_logs.c b/app/test/test_logs.c
index 05aa862..d0a9962 100644
--- a/app/test/test_logs.c
+++ b/app/test/test_logs.c
@@ -83,9 +83,6 @@ test_logs(void)
 	RTE_LOG(ERR, TESTAPP1, "error message\n");
 	RTE_LOG(ERR, TESTAPP2, "error message (not displayed)\n");
 
-	/* print again the previous logs */
-	rte_log_dump_history(stdout);
-
 	return 0;
 }
 
diff --git a/doc/guides/prog_guide/mempool_lib.rst b/doc/guides/prog_guide/mempool_lib.rst
index 5fae79a..c3afc2e 100644
--- a/doc/guides/prog_guide/mempool_lib.rst
+++ b/doc/guides/prog_guide/mempool_lib.rst
@@ -38,9 +38,7 @@ In the DPDK, it is identified by name and uses a ring to store free objects.
 It provides some other optional services such as a per-core object cache and
 an alignment helper to ensure that objects are padded to spread them equally on all DRAM or DDR3 channels.
 
-This library is used by the
-:ref:`Mbuf Library <Mbuf_Library>` and the
-:ref:`Environment Abstraction Layer <Environment_Abstraction_Layer>` (for logging history).
+This library is used by the :ref:`Mbuf Library <Mbuf_Library>`.
 
 Cookies
 -------
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ad05eba..bda40c1 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -8,6 +8,9 @@ API and ABI deprecation notices are to be posted here.
 Deprecation Notices
 -------------------
 
+* The log history is deprecated.
+  It is voided in 16.07 and will be removed in release 16.11.
+
 * The ethdev hotplug API is going to be moved to EAL with a notification
   mechanism added to crypto and ethdev libraries so that hotplug is now
   available to both of them. This API will be stripped of the device arguments
diff --git a/lib/librte_eal/bsdapp/eal/Makefile b/lib/librte_eal/bsdapp/eal/Makefile
index 9054ad6..474651b 100644
--- a/lib/librte_eal/bsdapp/eal/Makefile
+++ b/lib/librte_eal/bsdapp/eal/Makefile
@@ -41,7 +41,6 @@ CFLAGS += -I$(SRCDIR)/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common
 CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/include
 CFLAGS += -I$(RTE_SDK)/lib/librte_ring
-CFLAGS += -I$(RTE_SDK)/lib/librte_mempool
 CFLAGS += $(WERROR_FLAGS) -O3
 
 LDLIBS += -lexecinfo
diff --git a/lib/librte_eal/bsdapp/eal/eal_debug.c b/lib/librte_eal/bsdapp/eal/eal_debug.c
index 907fbfa..5fbc17c 100644
--- a/lib/librte_eal/bsdapp/eal/eal_debug.c
+++ b/lib/librte_eal/bsdapp/eal/eal_debug.c
@@ -77,9 +77,6 @@ void __rte_panic(const char *funcname, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	rte_log(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, "PANIC in %s():\n", funcname);
 	va_start(ap, format);
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
@@ -98,9 +95,6 @@ rte_exit(int exit_code, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	if (exit_code != 0)
 		RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
 				"  Cause: ", exit_code);
diff --git a/lib/librte_eal/common/eal_common_log.c b/lib/librte_eal/common/eal_common_log.c
index b5e37bb..7916c78 100644
--- a/lib/librte_eal/common/eal_common_log.c
+++ b/lib/librte_eal/common/eal_common_log.c
@@ -31,54 +31,16 @@
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <string.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdarg.h>
-#include <sys/types.h>
 #include <stdlib.h>
-#include <unistd.h>
-#include <inttypes.h>
-#include <errno.h>
-#include <sys/queue.h>
 
 #include <rte_log.h>
-#include <rte_memory.h>
-#include <rte_memzone.h>
-#include <rte_launch.h>
-#include <rte_common.h>
-#include <rte_cycles.h>
-#include <rte_eal.h>
 #include <rte_per_lcore.h>
-#include <rte_lcore.h>
-#include <rte_atomic.h>
-#include <rte_debug.h>
-#include <rte_spinlock.h>
-#include <rte_branch_prediction.h>
-#include <rte_ring.h>
-#include <rte_mempool.h>
 
 #include "eal_private.h"
 
-#define LOG_ELT_SIZE     2048
-
-#define LOG_HISTORY_MP_NAME "log_history"
-
-STAILQ_HEAD(log_history_list, log_history);
-
-/**
- * The structure of a message log in the log history.
- */
-struct log_history {
-	STAILQ_ENTRY(log_history) next;
-	unsigned size;
-	char buf[0];
-};
-
-static struct rte_mempool *log_history_mp = NULL;
-static unsigned log_history_size = 0;
-static struct log_history_list log_history;
-
 /* global log structure */
 struct rte_logs rte_logs = {
 	.type = ~0,
@@ -86,10 +48,7 @@ struct rte_logs rte_logs = {
 	.file = NULL,
 };
 
-static rte_spinlock_t log_dump_lock = RTE_SPINLOCK_INITIALIZER;
-static rte_spinlock_t log_list_lock = RTE_SPINLOCK_INITIALIZER;
 static FILE *default_log_stream;
-static int history_enabled = 1;
 
 /**
  * This global structure stores some informations about the message
@@ -106,59 +65,16 @@ static RTE_DEFINE_PER_LCORE(struct log_cur_msg, log_cur_msg);
 /* default logs */
 
 int
-rte_log_add_in_history(const char *buf, size_t size)
+rte_log_add_in_history(const char *buf __rte_unused, size_t size __rte_unused)
 {
-	struct log_history *hist_buf = NULL;
-	static const unsigned hist_buf_size = LOG_ELT_SIZE - sizeof(*hist_buf);
-	void *obj;
-
-	if (history_enabled == 0)
-		return 0;
-
-	rte_spinlock_lock(&log_list_lock);
-
-	/* get a buffer for adding in history */
-	if (log_history_size > RTE_LOG_HISTORY) {
-		hist_buf = STAILQ_FIRST(&log_history);
-		if (hist_buf) {
-			STAILQ_REMOVE_HEAD(&log_history, next);
-			log_history_size--;
-		}
-	}
-	else {
-		if (rte_mempool_mc_get(log_history_mp, &obj) < 0)
-			obj = NULL;
-		hist_buf = obj;
-	}
-
-	/* no buffer */
-	if (hist_buf == NULL) {
-		rte_spinlock_unlock(&log_list_lock);
-		return -ENOBUFS;
-	}
-
-	/* not enough room for msg, buffer go back in mempool */
-	if (size >= hist_buf_size) {
-		rte_mempool_mp_put(log_history_mp, hist_buf);
-		rte_spinlock_unlock(&log_list_lock);
-		return -ENOBUFS;
-	}
-
-	/* add in history */
-	memcpy(hist_buf->buf, buf, size);
-	hist_buf->buf[size] = hist_buf->buf[hist_buf_size-1] = '\0';
-	hist_buf->size = size;
-	STAILQ_INSERT_TAIL(&log_history, hist_buf, next);
-	log_history_size++;
-	rte_spinlock_unlock(&log_list_lock);
-
 	return 0;
 }
 
 void
 rte_log_set_history(int enable)
 {
-	history_enabled = enable;
+	if (enable)
+		RTE_LOG(WARNING, EAL, "The log history is deprecated.\n");
 }
 
 /* Change the stream that will be used by logging system */
@@ -217,44 +133,8 @@ int rte_log_cur_msg_logtype(void)
 
 /* Dump log history to file */
 void
-rte_log_dump_history(FILE *out)
+rte_log_dump_history(FILE *out __rte_unused)
 {
-	struct log_history_list tmp_log_history;
-	struct log_history *hist_buf;
-	unsigned i;
-
-	/* only one dump at a time */
-	rte_spinlock_lock(&log_dump_lock);
-
-	/* save list, and re-init to allow logging during dump */
-	rte_spinlock_lock(&log_list_lock);
-	tmp_log_history = log_history;
-	STAILQ_INIT(&log_history);
-	log_history_size = 0;
-	rte_spinlock_unlock(&log_list_lock);
-
-	for (i=0; i<RTE_LOG_HISTORY; i++) {
-
-		/* remove one message from history list */
-		hist_buf = STAILQ_FIRST(&tmp_log_history);
-
-		if (hist_buf == NULL)
-			break;
-
-		STAILQ_REMOVE_HEAD(&tmp_log_history, next);
-
-		/* write on stdout */
-		if (fwrite(hist_buf->buf, hist_buf->size, 1, out) == 0) {
-			rte_mempool_mp_put(log_history_mp, hist_buf);
-			break;
-		}
-
-		/* put back message structure in pool */
-		rte_mempool_mp_put(log_history_mp, hist_buf);
-	}
-	fflush(out);
-
-	rte_spinlock_unlock(&log_dump_lock);
 }
 
 /*
@@ -297,29 +177,11 @@ rte_log(uint32_t level, uint32_t logtype, const char *format, ...)
 }
 
 /*
- * called by environment-specific log init function to initialize log
- * history
+ * called by environment-specific log init function
  */
 int
 rte_eal_common_log_init(FILE *default_log)
 {
-	STAILQ_INIT(&log_history);
-
-	/* reserve RTE_LOG_HISTORY*2 elements, so we can dump and
-	 * keep logging during this time */
-	log_history_mp = rte_mempool_create(LOG_HISTORY_MP_NAME, RTE_LOG_HISTORY*2,
-				LOG_ELT_SIZE, 0, 0,
-				NULL, NULL,
-				NULL, NULL,
-				SOCKET_ID_ANY, MEMPOOL_F_NO_PHYS_CONTIG);
-
-	if ((log_history_mp == NULL) &&
-	    ((log_history_mp = rte_mempool_lookup(LOG_HISTORY_MP_NAME)) == NULL)){
-		RTE_LOG(ERR, EAL, "%s(): cannot create log_history mempool\n",
-			__func__);
-		return -1;
-	}
-
 	default_log_stream = default_log;
 	rte_openlog_stream(default_log);
 
diff --git a/lib/librte_eal/common/eal_private.h b/lib/librte_eal/common/eal_private.h
index 2342fa1..857dc3e 100644
--- a/lib/librte_eal/common/eal_private.h
+++ b/lib/librte_eal/common/eal_private.h
@@ -49,9 +49,6 @@ int rte_eal_memzone_init(void);
 /**
  * Common log initialization function (private to eal).
  *
- * Called by environment-specific log initialization function to initialize
- * log history.
- *
  * @param default_log
  *   The default log stream to be used.
  * @return
diff --git a/lib/librte_eal/common/include/rte_log.h b/lib/librte_eal/common/include/rte_log.h
index 2e47e7f..b1add04 100644
--- a/lib/librte_eal/common/include/rte_log.h
+++ b/lib/librte_eal/common/include/rte_log.h
@@ -42,6 +42,8 @@
  * This file provides a log API to RTE applications.
  */
 
+#include "rte_common.h" /* for __rte_deprecated macro */
+
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -179,22 +181,27 @@ int rte_log_cur_msg_loglevel(void);
 int rte_log_cur_msg_logtype(void);
 
 /**
+ * @deprecated
  * Enable or disable the history (enabled by default)
  *
  * @param enable
  *   true to enable, or 0 to disable history.
  */
+__rte_deprecated
 void rte_log_set_history(int enable);
 
 /**
+ * @deprecated
  * Dump the log history to a file
  *
  * @param f
  *   A pointer to a file for output
  */
+__rte_deprecated
 void rte_log_dump_history(FILE *f);
 
 /**
+ * @deprecated
  * Add a log message to the history.
  *
  * This function can be called from a user-defined log stream. It adds
@@ -209,6 +216,7 @@ void rte_log_dump_history(FILE *f);
  *   - 0: Success.
  *   - (-ENOBUFS) if there is no room to store the message.
  */
+__rte_deprecated
 int rte_log_add_in_history(const char *buf, size_t size);
 
 /**
diff --git a/lib/librte_eal/linuxapp/eal/eal_debug.c b/lib/librte_eal/linuxapp/eal/eal_debug.c
index 907fbfa..5fbc17c 100644
--- a/lib/librte_eal/linuxapp/eal/eal_debug.c
+++ b/lib/librte_eal/linuxapp/eal/eal_debug.c
@@ -77,9 +77,6 @@ void __rte_panic(const char *funcname, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	rte_log(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, "PANIC in %s():\n", funcname);
 	va_start(ap, format);
 	rte_vlog(RTE_LOG_CRIT, RTE_LOGTYPE_EAL, format, ap);
@@ -98,9 +95,6 @@ rte_exit(int exit_code, const char *format, ...)
 {
 	va_list ap;
 
-	/* disable history */
-	rte_log_set_history(0);
-
 	if (exit_code != 0)
 		RTE_LOG(CRIT, EAL, "Error - exiting with code: %d\n"
 				"  Cause: ", exit_code);
diff --git a/lib/librte_eal/linuxapp/eal/eal_log.c b/lib/librte_eal/linuxapp/eal/eal_log.c
index 0b133c3..d391100 100644
--- a/lib/librte_eal/linuxapp/eal/eal_log.c
+++ b/lib/librte_eal/linuxapp/eal/eal_log.c
@@ -50,8 +50,7 @@
 #include "eal_private.h"
 
 /*
- * default log function, used once mempool (hence log history) is
- * available
+ * default log function
  */
 static ssize_t
 console_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
@@ -60,9 +59,6 @@ console_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
 	ssize_t ret;
 	uint32_t loglevel;
 
-	/* add this log in history */
-	rte_log_add_in_history(buf, size);
-
 	/* write on stdout */
 	ret = fwrite(buf, 1, size, stdout);
 	fflush(stdout);
@@ -110,8 +106,7 @@ rte_eal_log_init(const char *id, int facility)
 /* early logs */
 
 /*
- * early log function, used during boot when mempool (hence log
- * history) is not available
+ * early log function, used before rte_eal_log_init
  */
 static ssize_t
 early_log_write(__attribute__((unused)) void *c, const char *buf, size_t size)
diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
index b54de43..22a5645 100644
--- a/lib/librte_mempool/rte_mempool.c
+++ b/lib/librte_mempool/rte_mempool.c
@@ -1003,7 +1003,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 
 		if (free == 0) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE1) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1012,7 +1011,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 			hdr->cookie = RTE_MEMPOOL_HEADER_COOKIE2;
 		} else if (free == 1) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE2) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1022,7 +1020,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 		} else if (free == 2) {
 			if (cookie != RTE_MEMPOOL_HEADER_COOKIE1 &&
 			    cookie != RTE_MEMPOOL_HEADER_COOKIE2) {
-				rte_log_set_history(0);
 				RTE_LOG(CRIT, MEMPOOL,
 					"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 					obj, (const void *) mp, cookie);
@@ -1032,7 +1029,6 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
 		tlr = __mempool_get_trailer(obj);
 		cookie = tlr->cookie;
 		if (cookie != RTE_MEMPOOL_TRAILER_COOKIE) {
-			rte_log_set_history(0);
 			RTE_LOG(CRIT, MEMPOOL,
 				"obj=%p, mempool=%p, cookie=%" PRIx64 "\n",
 				obj, (const void *) mp, cookie);
-- 
2.7.0

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Burakov, Anatoly]

Hi Thomas,

Just a few notes:

> 3/ The automatic mapped allocation of DPDK objects in the guest.
> It should not be done in EAL.
> An ivshmem driver would be called by rte_eal_dev_init.
> It would check where are the shared DPDK structures, as currently done with
> the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate allocations.
> Thus only the driver would depend on ring and mempool.

The problem here is IVSHMEM doesn't allocate the memory from DPDK, it allocates new memory segments by mapping a PCI device. I.e. it doesn't do mallocs, it modifies mem_config and adds memory to DPDK. Can that be done from within a PMD?

> 
> The last step of the ivshmem cleanup will be to remove the memory hack
> RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM
> could be removed.

The reason for that hack is that we often need to map several hugepages, and some of those pages could be 2M in size. If you're sharing 1G worth of contiguous memory backed by 2M pages, that's 512 files in the command line in vanilla DPDK, but can be made into one with RTE_EAL_SINGLE_FILE_SEGMENTS, so that QEMU command-line doesn't get overly long.

So removing this hack, while definitely desired, will adversely affect some use cases, such as using IVSHMEM on platforms where 1G pages aren't supported. Whether we want to go with the effort of supporting those is of course an open question - I personally don't have any data on IVSHMEM userbase. Maybe Kevin/other OVS devs could help me out here :)

Thanks,
Anatoly

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Thomas Monjalon]

2016-06-10 09:05, Burakov, Anatoly:
> Hi Thomas,
> 
> Just a few notes:
> 
> > 3/ The automatic mapped allocation of DPDK objects in the guest.
> > It should not be done in EAL.
> > An ivshmem driver would be called by rte_eal_dev_init.
> > It would check where are the shared DPDK structures, as currently done with
> > the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate allocations.
> > Thus only the driver would depend on ring and mempool.
> 
> The problem here is IVSHMEM doesn't allocate the memory from DPDK, it allocates new memory segments by mapping a PCI device. I.e. it doesn't do mallocs, it modifies mem_config and adds memory to DPDK. Can that be done from within a PMD?

Everything is possible :)
Maybe you just need to add an API to add some memory segments.
Other question: why is it so important to register these memory segments
in EAL? I think they just need to be known by the ivshmem driver which
map some objects on top.

> > The last step of the ivshmem cleanup will be to remove the memory hack
> > RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM
> > could be removed.
> 
> The reason for that hack is that we often need to map several hugepages, and some of those pages could be 2M in size. If you're sharing 1G worth of contiguous memory backed by 2M pages, that's 512 files in the command line in vanilla DPDK, but can be made into one with RTE_EAL_SINGLE_FILE_SEGMENTS, so that QEMU command-line doesn't get overly long.
> 
> So removing this hack, while definitely desired, will adversely affect some use cases, such as using IVSHMEM on platforms where 1G pages aren't supported. Whether we want to go with the effort of supporting those is of course an open question - I personally don't have any data on IVSHMEM userbase. Maybe Kevin/other OVS devs could help me out here :)

We can keep supporting 2M pages by having a command line option,
instead of the #ifdef RTE_EAL_SINGLE_FILE_SEGMENTS.
But as I said, it is not the top priority to remove this hack.

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Burakov, Anatoly]

> > Hi Thomas,
> >
> > Just a few notes:
> >
> > > 3/ The automatic mapped allocation of DPDK objects in the guest.
> > > It should not be done in EAL.
> > > An ivshmem driver would be called by rte_eal_dev_init.
> > > It would check where are the shared DPDK structures, as currently
> > > done with the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate
> allocations.
> > > Thus only the driver would depend on ring and mempool.
> >
> > The problem here is IVSHMEM doesn't allocate the memory from DPDK, it
> allocates new memory segments by mapping a PCI device. I.e. it doesn't do
> mallocs, it modifies mem_config and adds memory to DPDK. Can that be
> done from within a PMD?
> 
> Everything is possible :)
> Maybe you just need to add an API to add some memory segments.
> Other question: why is it so important to register these memory segments in
> EAL? I think they just need to be known by the ivshmem driver which map
> some objects on top.

That's because we need the memzone_lookup functionality. We can get by without it with rings because those are tailq-based, so we can just put rings there, but memzones are looked up through the memconfig, so IVSHMEM memzones have to be present there in order for the code to work without any additional API's.

Although, I guess we don't really need to have _memsegs_ in order to lookup memzones - we just have to create some memzones directly inside mcfg, bypassing the normal memzone_reserve stuff. That would still be a hack, but probably much less of a hack than what there is right now :) 

Another possible issue here is the order in which the memory is allocated. We put IVSHMEM init in EAL because we have to map things at specific addresses. The later IVSHMEM initializes, the more chance something will take up memory space that IVSHMEM needs. This could probably be solved with --base-virtaddr, so documentation will have to be updated to include advice to use that flag.

> 
> > > The last step of the ivshmem cleanup will be to remove the memory
> > > hack RTE_EAL_SINGLE_FILE_SEGMENTS. Then
> CONFIG_RTE_LIBRTE_IVSHMEM
> > > could be removed.
> >
> > The reason for that hack is that we often need to map several hugepages,
> and some of those pages could be 2M in size. If you're sharing 1G worth of
> contiguous memory backed by 2M pages, that's 512 files in the command line
> in vanilla DPDK, but can be made into one with
> RTE_EAL_SINGLE_FILE_SEGMENTS, so that QEMU command-line doesn't get
> overly long.
> >
> > So removing this hack, while definitely desired, will adversely affect
> > some use cases, such as using IVSHMEM on platforms where 1G pages
> > aren't supported. Whether we want to go with the effort of supporting
> > those is of course an open question - I personally don't have any data
> > on IVSHMEM userbase. Maybe Kevin/other OVS devs could help me out
> here
> > :)
> 
> We can keep supporting 2M pages by having a command line option, instead
> of the #ifdef RTE_EAL_SINGLE_FILE_SEGMENTS.
> But as I said, it is not the top priority to remove this hack.

Ah, so you're not suggesting removing the _functionality_, just the #ifdef? That could be made to work I guess...

Also, please correct me if I'm wrong, but I seem to remember some patches about putting all memory in a single file - I think that should work for IVSHMEM as well, because I believe IVSHMEM handles holes in files just fine, and can map even if everything resides inside a single file. So if that patch does what I think it does, we might just integrate it and remove the single file segments code entirely.

Thanks,
Anatoly

Re: [PATCH v3] log: deprecate history dump

[David Marchand]

On Fri, Jun 10, 2016 at 12:10 AM, Thomas Monjalon
<thomas.monjalon@6wind.com> wrote:
> The log history uses rte_mempool. In order to remove the mempool
> dependency in EAL (and improve the build), this feature is deprecated.
> The ABI is kept but the behaviour is now voided because it seems this
> function was not used. The history can be read from syslog.
>
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Acked-by: David Marchand <david.marchand@6wind.com>


-- 
David Marchand

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Thomas Monjalon]

2016-06-10 09:47, Burakov, Anatoly:
> > > > The last step of the ivshmem cleanup will be to remove the memory
> > > > hack RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM
> > > > could be removed.
> > >
> > > The reason for that hack is that we often need to map several hugepages,
> > and some of those pages could be 2M in size. If you're sharing 1G worth of
> > contiguous memory backed by 2M pages, that's 512 files in the command line
> > in vanilla DPDK, but can be made into one with
> > RTE_EAL_SINGLE_FILE_SEGMENTS, so that QEMU command-line doesn't get
> > overly long.
> > >
> > > So removing this hack, while definitely desired, will adversely affect
> > > some use cases, such as using IVSHMEM on platforms where 1G pages
> > > aren't supported. Whether we want to go with the effort of supporting
> > > those is of course an open question - I personally don't have any data
> > > on IVSHMEM userbase. Maybe Kevin/other OVS devs could help me out
> > here
> > > :)
> > 
> > We can keep supporting 2M pages by having a command line option, instead
> > of the #ifdef RTE_EAL_SINGLE_FILE_SEGMENTS.
> > But as I said, it is not the top priority to remove this hack.
> 
> Ah, so you're not suggesting removing the _functionality_, just the #ifdef? That could be made to work I guess...
> 
> Also, please correct me if I'm wrong, but I seem to remember some patches about putting all memory in a single file - I think that should work for IVSHMEM as well, because I believe IVSHMEM handles holes in files just fine, and can map even if everything resides inside a single file. So if that patch does what I think it does, we might just integrate it and remove the single file segments code entirely.

Yes it can be considered in a memory allocator rework.
Please jump in this thread:
	http://dpdk.org/ml/archives/dev/2016-April/037444.html

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Burakov, Anatoly]

Hi Thomas, 

> 2016-06-10 09:47, Burakov, Anatoly:
> > > > > The last step of the ivshmem cleanup will be to remove the
> > > > > memory hack RTE_EAL_SINGLE_FILE_SEGMENTS. Then
> > > > > CONFIG_RTE_LIBRTE_IVSHMEM could be removed.
> > > >
> > > > The reason for that hack is that we often need to map several
> > > > hugepages,
> > > and some of those pages could be 2M in size. If you're sharing 1G
> > > worth of contiguous memory backed by 2M pages, that's 512 files in
> > > the command line in vanilla DPDK, but can be made into one with
> > > RTE_EAL_SINGLE_FILE_SEGMENTS, so that QEMU command-line doesn't
> get
> > > overly long.
> > > >
> > > > So removing this hack, while definitely desired, will adversely
> > > > affect some use cases, such as using IVSHMEM on platforms where 1G
> > > > pages aren't supported. Whether we want to go with the effort of
> > > > supporting those is of course an open question - I personally
> > > > don't have any data on IVSHMEM userbase. Maybe Kevin/other OVS
> > > > devs could help me out
> > > here
> > > > :)
> > >
> > > We can keep supporting 2M pages by having a command line option,
> > > instead of the #ifdef RTE_EAL_SINGLE_FILE_SEGMENTS.
> > > But as I said, it is not the top priority to remove this hack.
> >
> > Ah, so you're not suggesting removing the _functionality_, just the #ifdef?
> That could be made to work I guess...
> >
> > Also, please correct me if I'm wrong, but I seem to remember some
> patches about putting all memory in a single file - I think that should work for
> IVSHMEM as well, because I believe IVSHMEM handles holes in files just fine,
> and can map even if everything resides inside a single file. So if that patch
> does what I think it does, we might just integrate it and remove the single file
> segments code entirely.
> 
> Yes it can be considered in a memory allocator rework.
> Please jump in this thread:
> 	http://dpdk.org/ml/archives/dev/2016-April/037444.html

I've read through that thread, and I don't think anything should be done to support that in IVSHMEM. IVSHMEM library should correctly detect all mapped files, whatever the memory layout on the host side (contiguous, noncontiguous, single file...). On the guest, we're simply adding memzones which we map from a PCI device, so IVSHMEM shouldn't be affected by any changes to how memzones/mempools are allocated (mempools aren't even allowed for use with IVSHMEM).

Thanks,
Anatoly

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Thomas Monjalon]

2016-06-10 12:08, Burakov, Anatoly:
> Hi Thomas, 
> 
> > 2016-06-10 09:47, Burakov, Anatoly:
> > > > > > The last step of the ivshmem cleanup will be to remove the
> > > > > > memory hack RTE_EAL_SINGLE_FILE_SEGMENTS. Then
> > > > > > CONFIG_RTE_LIBRTE_IVSHMEM could be removed.
> > > > >
> > > > > The reason for that hack is that we often need to map several
> > > > > hugepages,
> > > > and some of those pages could be 2M in size. If you're sharing 1G
> > > > worth of contiguous memory backed by 2M pages, that's 512 files in
> > > > the command line in vanilla DPDK, but can be made into one with
> > > > RTE_EAL_SINGLE_FILE_SEGMENTS, so that QEMU command-line doesn't
> > get
> > > > overly long.
> > > > >
> > > > > So removing this hack, while definitely desired, will adversely
> > > > > affect some use cases, such as using IVSHMEM on platforms where 1G
> > > > > pages aren't supported. Whether we want to go with the effort of
> > > > > supporting those is of course an open question - I personally
> > > > > don't have any data on IVSHMEM userbase. Maybe Kevin/other OVS
> > > > > devs could help me out
> > > > here
> > > > > :)
> > > >
> > > > We can keep supporting 2M pages by having a command line option,
> > > > instead of the #ifdef RTE_EAL_SINGLE_FILE_SEGMENTS.
> > > > But as I said, it is not the top priority to remove this hack.
> > >
> > > Ah, so you're not suggesting removing the _functionality_, just the #ifdef?
> > That could be made to work I guess...
> > >
> > > Also, please correct me if I'm wrong, but I seem to remember some
> > patches about putting all memory in a single file - I think that should work for
> > IVSHMEM as well, because I believe IVSHMEM handles holes in files just fine,
> > and can map even if everything resides inside a single file. So if that patch
> > does what I think it does, we might just integrate it and remove the single file
> > segments code entirely.
> > 
> > Yes it can be considered in a memory allocator rework.
> > Please jump in this thread:
> > 	http://dpdk.org/ml/archives/dev/2016-April/037444.html
> 
> I've read through that thread, and I don't think anything should be done to support that in IVSHMEM. IVSHMEM library should correctly detect all mapped files, whatever the memory layout on the host side (contiguous, noncontiguous, single file...). On the guest, we're simply adding memzones which we map from a PCI device, so IVSHMEM shouldn't be affected by any changes to how memzones/mempools are allocated (mempools aren't even allowed for use with IVSHMEM).

You misunderstood me. I was talking about a rework of
RTE_EAL_SINGLE_FILE_SEGMENTS. I just say it must be reworked and
it can happen in the discussion of a global rework of the memory allocator.
It should not be related to IVSHMEM.

Re: [PATCH v3] log: deprecate history dump

[Thomas Monjalon]

> > The log history uses rte_mempool. In order to remove the mempool
> > dependency in EAL (and improve the build), this feature is deprecated.
> > The ABI is kept but the behaviour is now voided because it seems this
> > function was not used. The history can be read from syslog.
> >
> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> 
> Acked-by: David Marchand <david.marchand@6wind.com>

Applied

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Ferruh Yigit]

On 6/9/2016 10:26 PM, Thomas Monjalon wrote:
> Looking a bit more into librte_ivshmem, the documentation says we need
> a Qemu patch but the URL doesn't exist anymore:
> 	https://01.org/packet-processing/intel%C2%AE-ovdk
> 	-> 404 Oops, we couldn't find that page
> 
> I've never understood why we should keep this wart and now I'm going
> to be upset.
> To sum up the situation, eal depends on ivshmem which depends on
> ring/mempool which depends... on eal.
> The truth is that eal should not depends on librte_ivshmem.
> And the option CONFIG_RTE_LIBRTE_IVSHMEM should not exist.
> 
> There are 3 parts to distinguish:
> 
> 1/ The librte_ivshmem API to export some data structures from host.
> No real problem here.
> 
> 2/ The scan of the ivshmem devices in the guest init.
> It should be handled as any other PCI device with an appropriate driver.
> The scan is done by rte_eal_pci_init.
> 
> 3/ The automatic mapped allocation of DPDK objects in the guest.
> It should not be done in EAL.
> An ivshmem driver would be called by rte_eal_dev_init.
> It would check where are the shared DPDK structures, as currently done
> with the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate allocations.
> Thus only the driver would depend on ring and mempool.
> 
> The last step of the ivshmem cleanup will be to remove the memory hack
> RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM could be
> removed.
> 
> So this is my proposal:
> Someone start working on the above cleanup now, otherwise the whole
> rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.

I would like to note that at least SPP (soft patch panel) is using
rte_ivshmem feature.

> We already talked about the rte_ivshmem design issues several times
> and nobody declared using it.
> 

Re: [PATCH] dropping librte_ivshmem

[Thomas Monjalon]

2016-06-15 19:16, Ferruh Yigit:
> On 6/9/2016 10:26 PM, Thomas Monjalon wrote:
> > So this is my proposal:
> > Someone start working on the above cleanup now, otherwise the whole
> > rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.
> 
> I would like to note that at least SPP (soft patch panel) is using
> rte_ivshmem feature.

Why?
Can SPP work without ivshmem?

Re: [PATCH] dropping librte_ivshmem

[Ferruh Yigit]

On 6/15/2016 7:34 PM, Thomas Monjalon wrote:
> 2016-06-15 19:16, Ferruh Yigit:
>> On 6/9/2016 10:26 PM, Thomas Monjalon wrote:
>>> So this is my proposal:
>>> Someone start working on the above cleanup now, otherwise the whole
>>> rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.
>>
>> I would like to note that at least SPP (soft patch panel) is using
>> rte_ivshmem feature.
> 
> Why?
> Can SPP work without ivshmem?
> 

Can do, it supports both ivshmem and vhost.
But ivshmem gives clearly more performance for VM to VM communication.

Re: [PATCH] dropping librte_ivshmem

[Thomas Monjalon]

2016-06-20 16:44, Ferruh Yigit:
> On 6/15/2016 7:34 PM, Thomas Monjalon wrote:
> > 2016-06-15 19:16, Ferruh Yigit:
> >> On 6/9/2016 10:26 PM, Thomas Monjalon wrote:
> >>> So this is my proposal:
> >>> Someone start working on the above cleanup now, otherwise the whole
> >>> rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.
> >>
> >> I would like to note that at least SPP (soft patch panel) is using
> >> rte_ivshmem feature.
> > 
> > Why?
> > Can SPP work without ivshmem?
> > 
> 
> Can do, it supports both ivshmem and vhost.
> But ivshmem gives clearly more performance for VM to VM communication.

But SPP is not about performance? My understanding is that it supports
what DPDK supports.
And the question here is not about the benefit of QEMU ivshmem but the
current design of DPDK object sharing via ivshmem (librte_ivshmem).

Re: [PATCH] dropping librte_ivshmem - was log: deprecate history dump

[Panu Matilainen]

On 06/10/2016 12:26 AM, Thomas Monjalon wrote:
> Looking a bit more into librte_ivshmem, the documentation says we need
> a Qemu patch but the URL doesn't exist anymore:
> 	https://01.org/packet-processing/intel%C2%AE-ovdk
> 	-> 404 Oops, we couldn't find that page
>
> I've never understood why we should keep this wart and now I'm going
> to be upset.

Good :)

> To sum up the situation, eal depends on ivshmem which depends on
> ring/mempool which depends... on eal.
> The truth is that eal should not depends on librte_ivshmem.
> And the option CONFIG_RTE_LIBRTE_IVSHMEM should not exist.
>
> There are 3 parts to distinguish:
>
> 1/ The librte_ivshmem API to export some data structures from host.
> No real problem here.
>
> 2/ The scan of the ivshmem devices in the guest init.
> It should be handled as any other PCI device with an appropriate driver.
> The scan is done by rte_eal_pci_init.
>
> 3/ The automatic mapped allocation of DPDK objects in the guest.
> It should not be done in EAL.
> An ivshmem driver would be called by rte_eal_dev_init.
> It would check where are the shared DPDK structures, as currently done
> with the IVSHMEM_MAGIC (0x0BADC0DE), and do the appropriate allocations.
> Thus only the driver would depend on ring and mempool.
>
> The last step of the ivshmem cleanup will be to remove the memory hack
> RTE_EAL_SINGLE_FILE_SEGMENTS. Then CONFIG_RTE_LIBRTE_IVSHMEM could be
> removed.
>
> So this is my proposal:
> Someone start working on the above cleanup now, otherwise the whole
> rte_ivshmem feature will be deprecated in 16.07 and removed in 16.11.
> We already talked about the rte_ivshmem design issues several times
> and nobody declared using it.

+1 (more like +100) to that.

In addition to the technical mess in EAL, there are quite some 
eyebrow-raisers related to IVSHMEM:

That it all starts with "you'll need to build a special version of qemu" 
with this special patch from the 'net, a patch which doesn't even exist 
anymore, is a complete non-starter. Such a situation can occur during 
early development, but its been years by now. Dependencies to 
non-upstreamed features in other projects are not a healthy sign.

Regardless of whether the patch has been integrated to qemu upstream or 
not, the situation is quite telling: nobody cares enough to have updated 
the information. I found a copy of the patch from my laptop, and as far 
as I can tell, the patch has never been proposed upstream, much less 
applied. Certainly the patch would not come even close to applying to 
current qemu. And apparently IVSHMEM is unmaintained in qemu upstream 
too (according to MAINTAINERS).

On DPDK side, that the most obvious (to me at least) user of memnic PMD 
has been unmaintained for two years no, and allowed to fall off the edge 
of the world (witness http://dpdk.org/browse/memnic/) is also quite telling.

Just deprecate it already. If somebody shows up with actual patches to 
clean it all up, the deprecation can be lifted of course, but cleaning 
up this abandonware seems like waste of engineering resources to me.

	- Panu -