From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f45.google.com (mail-wg0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id 08FF81288 for ; Mon, 1 Jun 2015 11:35:32 +0200 (CEST) Received: by wgbgq6 with SMTP id gq6so109116746wgb.3 for ; Mon, 01 Jun 2015 02:35:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=TBxR4ReY4aebNIruSHUkwpPFH8rzui5MvVTS1ZjMrFg=; b=s1ZcZg+Ry+qwGOKZXndm2OOZiCvKWTul5TutxWMSMYLiGDibOJtAdpExA7WMcwPqpF 3xDdB7YdDEjlKw5wd81pSik2/nOzs0XZBUAWzcMC64IqLVn4USya1n3vPvem5t9NwNOd bdEtAEPFSUwKkHbJmrNBVf2yQpQ7aV1K+7DEWMiYvS+HSCFzQDhI9KuaQMnlny16QVSP +FrHRsSk8cEs6YyOSkuDkhAGN4gr2QIvaF8a9blqv8VFSpSf8VAgWz+2tDhNKOBwI1Wz 9j4carQQVGU8tDuWEFnN6wkcsSwMEFeBdHJQ+UB98DHtarE4EIrq1yAKz9k9nzu892iL iyvg== MIME-Version: 1.0 X-Received: by 10.194.90.100 with SMTP id bv4mr38174193wjb.143.1433151331921; Mon, 01 Jun 2015 02:35:31 -0700 (PDT) Sender: jblunck@gmail.com Received: by 10.195.12.101 with HTTP; Mon, 1 Jun 2015 02:35:31 -0700 (PDT) In-Reply-To: <556C1863.2080203@6wind.com> References: <1432895688-1728-1-git-send-email-jblunck@infradead.org> <556C1863.2080203@6wind.com> Date: Mon, 1 Jun 2015 11:35:31 +0200 X-Google-Sender-Auth: TuuyzLOF0ZLfwAiGkGepIVJ3I_U Message-ID: From: Jan Blunck To: Olivier MATZ Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] log: Properly reset log_history_size in rte_log_dump_history() X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Jun 2015 09:35:32 -0000 On Mon, Jun 1, 2015 at 10:31 AM, Olivier MATZ wrote: > Hi Jan, > > On 05/29/2015 12:34 PM, Jan Blunck wrote: > > In rte_log_dump_history() the log_history list is reinitialized without > > resetting the log_history_size. In the next call to > rte_log_add_in_history() > > the log_history_size > RTE_LOG_HISTORY and the code unconditionally tries > > to remove the first entry: > > > > Program received signal SIGSEGV, Segmentation fault. > > rte_log_add_in_history ( > > buf=buf@entry=0x7f02035cd000 "DATAPLANE: 9:dp0s7 link RTM_NEWLINK > [dp0s7] \nCAST,LOWER_UP>\n", > size=size@entry=86) > > at /usr/src/packages/BUILD/lib/librte_eal/common/eal_common_log.c:122 > > > > Signed-off-by: Jan Blunck > > --- > > lib/librte_eal/common/eal_common_log.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_eal/common/eal_common_log.c > b/lib/librte_eal/common/eal_common_log.c > > index fe3d7d5..cb4311c 100644 > > --- a/lib/librte_eal/common/eal_common_log.c > > +++ b/lib/librte_eal/common/eal_common_log.c > > @@ -119,7 +119,8 @@ rte_log_add_in_history(const char *buf, size_t size) > > /* get a buffer for adding in history */ > > if (log_history_size > RTE_LOG_HISTORY) { > > hist_buf = STAILQ_FIRST(&log_history); > > - STAILQ_REMOVE_HEAD(&log_history, next); > > + if (hist_buf) > > + STAILQ_REMOVE_HEAD(&log_history, next); > > Shouldn't we decrease log_history_size here? > > Thanks for catching that one. > > > Also, it's probably a bit off-topic, but I think the function that > adds in history could be optimized a bit to avoid doing the copy with > the lock held. Maybe something like this is feasible: > > rte_mempool_mc_get() into hist_buf > memcpy(hist_buf->buf, buf, size); > rte_spinlock_lock(&log_list_lock > if (log_history_size > RTE_LOG_HISTORY) { > STAILQ_REMOVE_HEAD > log_history_size -- > } > STAILQ_INSERT_TAIL > log_history_size ++ > rte_spinlock_unlock(&log_list_lock) > > Feel free to implement it if you feel it's better. It would also > require to increase the number of objects in the pool to > RTE_LOG_HISTORY*2 + RTE_MAX_LCORE > > Makes sense. I'll take a look into that one later. Thanks, Jan > Regards, > Olivier > > > > > } > > else { > > if (rte_mempool_mc_get(log_history_mp, &obj) < 0) > > @@ -234,6 +235,7 @@ rte_log_dump_history(FILE *out) > > 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 > >