Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933447AbcJRUh6 (ORCPT ); Tue, 18 Oct 2016 16:37:58 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:35304 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932511AbcJRUht (ORCPT ); Tue, 18 Oct 2016 16:37:49 -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: Tue, 18 Oct 2016 13:37:46 -0700 X-Google-Sender-Auth: dpZZdgo40IcoNgDwbFQpGoJbgj4 Message-ID: Subject: Re: [PATCH 5/7] ramoops: Split ftrace buffer space into per-CPU zones To: Joel Fernandes 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 Content-Length: 2566 Lines: 67 On Sun, Oct 16, 2016 at 10:40 AM, Joel Fernandes wrote: > 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. Ah-ha, okay. I wasn't sure if there was some way to grow nr_cpu_ids after boot. Thanks for checking! -Kees -- Kees Cook Nexus Security