Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752684AbcJJX7v (ORCPT ); Mon, 10 Oct 2016 19:59:51 -0400 Received: from mail-lf0-f54.google.com ([209.85.215.54]:33509 "EHLO mail-lf0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752354AbcJJX7u (ORCPT ); Mon, 10 Oct 2016 19:59:50 -0400 MIME-Version: 1.0 In-Reply-To: References: <1475904515-24970-1-git-send-email-joelaf@google.com> <1475904515-24970-6-git-send-email-joelaf@google.com> From: Kees Cook Date: Mon, 10 Oct 2016 16:59:47 -0700 X-Google-Sender-Auth: yoe-anyZO7oe2IpS7ivuVULwC6Q Message-ID: Subject: Re: [PATCH 5/7] ramoops: Split ftrace buffer space into per-CPU zones To: Joel Fernandes Cc: LKML , Steven Rostedt , Anton Vorontsov , Colin Cross , Tony Luck Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1967 Lines: 54 On Sun, Oct 9, 2016 at 10:15 AM, Joel Fernandes wrote: > On Fri, Oct 7, 2016 at 10:28 PM, Joel Fernandes wrote: >> If FTRACE_PER_CPU flag is passed to ramoops pdata, split the space into >> multiple zones depending on the number of CPUs. >> >> This speeds up the performance of function tracing by about 280% in my tests as >> we avoid the locking. The trade off being lesser space available per CPU. Let >> the ramoops user decide which option they want based on pdata flag. >> >> Signed-off-by: Joel Fernandes >> --- >> fs/pstore/ram.c | 70 +++++++++++++++++++++++++++++++++++----------- >> include/linux/pstore_ram.h | 3 ++ >> 2 files changed, 56 insertions(+), 17 deletions(-) >> >> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c > [..] >> @@ -391,14 +418,21 @@ static void ramoops_free_przs(struct ramoops_context *cxt) >> { >> int i; >> >> - if (!cxt->przs) >> - return; >> + /* Free dump PRZs */ >> + if (cxt->przs) { >> + for (i = 0; i < cxt->max_dump_cnt; i++) >> + persistent_ram_free(cxt->przs[i]); >> >> - for (i = 0; i < cxt->max_dump_cnt; i++) >> - persistent_ram_free(cxt->przs[i]); >> + kfree(cxt->przs); >> + cxt->max_dump_cnt = 0; >> + } >> >> - kfree(cxt->przs); >> - cxt->max_dump_cnt = 0; >> + /* Free ftrace PRZs */ >> + if (cxt->fprzs) { >> + for (i = 0; i < nr_cpu_ids; i++) >> + persistent_ram_free(cxt->przs[i]); > > I am supposed to free fprzs[i] here, instead of przs[i]. Also need to > check flag and free correct number of zones. Will fix for v2, sorry > about that. I think the cpu id needs to be bounds-checked against the size of the allocation. In theory, CPU hot-plug could grow the number of CPUs after pstore is initialized. -Kees -- Kees Cook Nexus Security