Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752765AbcJKAAz (ORCPT ); Mon, 10 Oct 2016 20:00:55 -0400 Received: from mail-lf0-f44.google.com ([209.85.215.44]:34058 "EHLO mail-lf0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752344AbcJKAAy (ORCPT ); Mon, 10 Oct 2016 20:00:54 -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 17:00:50 -0700 X-Google-Sender-Auth: PAPIHdrefPjEalUZvgBJlSZi2Vo 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: 2249 Lines: 59 On Mon, Oct 10, 2016 at 4:59 PM, Kees Cook wrote: > 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. Oh, also, this new flag needs to be explosed as a module parameter and Device Tree flag as well, so it's available for more than just the dummy driver. :) -Kees -- Kees Cook Nexus Security