[dpdk-dev] [PATCH] log: deprecate history dump

[David Marchand]

Thomas,

On Thu, Jun 9, 2016 at 4:09 PM, Thomas Monjalon
<thomas.monjalon at 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 at 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