From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757398AbcJPRlF (ORCPT ); Sun, 16 Oct 2016 13:41:05 -0400 Received: from mail-lf0-f48.google.com ([209.85.215.48]:33956 "EHLO mail-lf0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756953AbcJPRk7 (ORCPT ); Sun, 16 Oct 2016 13:40:59 -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: Joel Fernandes Date: Sun, 16 Oct 2016 10:40:56 -0700 Message-ID: Subject: Re: [PATCH 5/7] ramoops: Split ftrace buffer space into per-CPU zones To: Kees Cook Cc: LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kees, 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. nr_cpu_ids is fixed to the number of possible CPUs regardless of if hotplug is being used or not. I did a hotplug test as well to confirm this. So if I boot on 4 CPU machine with only 2 CPUs online, then nr_cpu_ids is 4. Thanks, Joel