From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <christian.ehrhardt@canonical.com>
Received: from mail-qg0-f53.google.com (mail-qg0-f53.google.com
 [209.85.192.53]) by dpdk.org (Postfix) with ESMTP id D3DE811D4
 for <dev@dpdk.org>; Thu,  9 Jun 2016 17:01:48 +0200 (CEST)
Received: by mail-qg0-f53.google.com with SMTP id v48so3177175qgd.2
 for <dev@dpdk.org>; Thu, 09 Jun 2016 08:01:48 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=canonical-com.20150623.gappssmtp.com; s=20150623;
 h=mime-version:in-reply-to:references:from:date:message-id:subject:to
 :cc; bh=SymFjg/mGxkhcEqFpTO0/jUrrMOfgMtW3DigTWmw2Po=;
 b=G/yNgZFAlT6Qf2QpWz3YZitNq9WTLouAxD5i7LmMrp2riWcFKXyBhnQSlm3Jn5bnfm
 Ou7o01Tb67/NW2Khu2cxRSyAkhiY36MB4j9Mev3RF62Xsc6VgW2J0bIEh26Xe5Xe8/EK
 YbadZm3fh+rtcQQWRp3QvYSq9xFG1i34u4unstZgBOOkQPquoWFLcIUe0V1VcR2b2e6A
 ELhSX9kxXS1IDbSaZcolAlQOEG3x6JmqdC9vmTT5W9zKvoMM8u18bARzQfCUvV0JYscL
 /ged/nrJQljp9Gdckn3qQwrA9EXqBhue7dGF4M+Nmq9QEXgwBtWmap4tA4M+fKYQR9P6
 M72Q==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20130820;
 h=x-gm-message-state:mime-version:in-reply-to:references:from:date
 :message-id:subject:to:cc;
 bh=SymFjg/mGxkhcEqFpTO0/jUrrMOfgMtW3DigTWmw2Po=;
 b=S2gkaIAtl31mJYE9MjW5W5Wo+HW4f+85pMDOEEicloBWhMY54w0aqz/eDObh/tkvaA
 OkNbbw6HDwpQr58inBqOFrTrGtYPIsqtxE6UDcdknjq6ertU1l5c5R4T7l2naldivtUG
 g5PlcNXYtSAvD+bRr1O16NyWsWpNYIJAvYdZpTsv49OQ2NCxtuJzzjjWy59Q+Z6+ar1p
 IoFJ0SSd75OPqyOVC5kQfPrYjdJBL7gSQ/bkAFUdim9cOw+V8kloaDf9z0mSrXLl02O2
 oDLuU+/IXh+Z12kMbCSGa8lQ81uafo/udKvoBuE3Tu/KFNQp48fRmCy28LeMITy8IWK+
 6vng==
X-Gm-Message-State: ALyK8tJpILB9RC4j1lJbWP7//qtfYmwEzJA6mLZYAy8oFzEM+2EDDzHbXz/mWYT9EJm0wsWDhq/fjFbfMEGwDEXJ
X-Received: by 10.140.40.239 with SMTP id x102mr1727974qgx.93.1465484508174;
 Thu, 09 Jun 2016 08:01:48 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.55.162.9 with HTTP; Thu, 9 Jun 2016 08:01:28 -0700 (PDT)
In-Reply-To: <CALwxeUurMwmeNNFcp__U7v_eaU0dyVLYgHXrg--JnS_qioYQHg@mail.gmail.com>
References: <1465481396-23968-1-git-send-email-thomas.monjalon@6wind.com>
 <CALwxeUurMwmeNNFcp__U7v_eaU0dyVLYgHXrg--JnS_qioYQHg@mail.gmail.com>
From: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Date: Thu, 9 Jun 2016 17:01:28 +0200
Message-ID: <CAATJJ0KjgxXa64DQv5aTnn-8oFKGqpS+=B3UX3w8GB42VKP25g@mail.gmail.com>
To: David Marchand <david.marchand@6wind.com>
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>
Content-Type: text/plain; charset=UTF-8
X-Content-Filtered-By: Mailman/MimeDel 2.1.15
Subject: Re: [dpdk-dev] [PATCH] log: deprecate history dump
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Thu, 09 Jun 2016 15:01:49 -0000

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
>