From: David Marchand <david.marchand@6wind.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] log: deprecate history dump
Date: Thu, 9 Jun 2016 16:45:44 +0200 [thread overview]
Message-ID: <CALwxeUurMwmeNNFcp__U7v_eaU0dyVLYgHXrg--JnS_qioYQHg@mail.gmail.com> (raw)
In-Reply-To: <1465481396-23968-1-git-send-email-thomas.monjalon@6wind.com>
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
next prev parent reply other threads:[~2016-06-09 14:46 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-09 14:09 Thomas Monjalon
2016-06-09 14:45 ` David Marchand [this message]
2016-06-09 15:01 ` Thomas Monjalon
2016-06-09 21:26 ` [dpdk-dev] [PATCH] dropping librte_ivshmem - was " Thomas Monjalon
2016-06-10 9:05 ` Burakov, Anatoly
2016-06-10 9:30 ` Thomas Monjalon
2016-06-10 9:47 ` Burakov, Anatoly
2016-06-10 10:13 ` Thomas Monjalon
2016-06-10 12:08 ` Burakov, Anatoly
2016-06-10 12:26 ` Thomas Monjalon
2016-06-15 18:16 ` Ferruh Yigit
2016-06-15 18:34 ` [dpdk-dev] [PATCH] dropping librte_ivshmem Thomas Monjalon
2016-06-20 15:44 ` Ferruh Yigit
2016-06-20 15:54 ` Thomas Monjalon
2016-06-21 6:49 ` [dpdk-dev] [PATCH] dropping librte_ivshmem - was log: deprecate history dump Panu Matilainen
2016-06-09 15:01 ` [dpdk-dev] [PATCH] " Christian Ehrhardt
2016-06-09 15:06 ` [dpdk-dev] [PATCH v2] " Thomas Monjalon
2016-06-09 22:10 ` [dpdk-dev] [PATCH v3] " Thomas Monjalon
2016-06-10 9:50 ` David Marchand
2016-06-10 13:09 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALwxeUurMwmeNNFcp__U7v_eaU0dyVLYgHXrg--JnS_qioYQHg@mail.gmail.com \
--to=david.marchand@6wind.com \
--cc=dev@dpdk.org \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).