From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754547AbdGNQNa (ORCPT ); Fri, 14 Jul 2017 12:13:30 -0400 Received: from mga11.intel.com ([192.55.52.93]:56407 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753522AbdGNQN3 (ORCPT ); Fri, 14 Jul 2017 12:13:29 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,359,1496127600"; d="scan'208";a="1151564190" Message-ID: <1500048805.29272.1.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: [PATCH 02/32] tracing: Reimplement log2 From: Tom Zanussi To: Namhyung Kim Cc: rostedt@goodmis.org, tglx@linutronix.de, mhiramat@kernel.org, vedang.patel@intel.com, linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org, kernel-team@lge.com Date: Fri, 14 Jul 2017 11:13:25 -0500 In-Reply-To: <20170714023321.GD6959@sejong> References: <032f7234ba8aef8428ed717f624e805e1d0e5524.1498510759.git.tom.zanussi@linux.intel.com> <20170714023321.GD6959@sejong> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Namhyung, On Fri, 2017-07-14 at 11:33 +0900, Namhyung Kim wrote: > On Mon, Jun 26, 2017 at 05:49:03PM -0500, Tom Zanussi wrote: > > log2 as currently implemented applies only to u64 trace_event_field > > derived fields, and assumes that anything it's applied to is a u64 > > field. > > > > To prepare for synthetic fields like latencies, log2 should be > > applicable to those as well, so take the opportunity now to fix the > > current problems as well as expand to more general uses. > > > > log2 should be thought of as a chaining function rather than a field > > type. To enable this as well as possible future function > > implementations, add a hist_field operand array into the hist_field > > definition for this purpose, and make use of it to implement the log2 > > 'function'. > > > > Signed-off-by: Tom Zanussi > > --- > > kernel/trace/trace_events_hist.c | 31 +++++++++++++++++++++++++++---- > > 1 file changed, 27 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > index 91ffc39..7b55956 100644 > > --- a/kernel/trace/trace_events_hist.c > > +++ b/kernel/trace/trace_events_hist.c > > @@ -28,12 +28,16 @@ > > > > typedef u64 (*hist_field_fn_t) (struct hist_field *field, void *event); > > > > +#define HIST_FIELD_OPERANDS_MAX 2 > > + > > struct hist_field { > > struct ftrace_event_field *field; > > unsigned long flags; > > hist_field_fn_t fn; > > unsigned int size; > > unsigned int offset; > > + unsigned int is_signed; > > + struct hist_field *operands[HIST_FIELD_OPERANDS_MAX]; > > }; > > > > static u64 hist_field_none(struct hist_field *field, void *event) > > @@ -71,7 +75,9 @@ static u64 hist_field_pstring(struct hist_field *hist_field, void *event) > > > > static u64 hist_field_log2(struct hist_field *hist_field, void *event) > > { > > - u64 val = *(u64 *)(event + hist_field->field->offset); > > + struct hist_field *operand = hist_field->operands[0]; > > + > > + u64 val = operand->fn(operand, event); > > > > return (u64) ilog2(roundup_pow_of_two(val)); > > } > > @@ -156,6 +162,8 @@ static const char *hist_field_name(struct hist_field *field, > > > > if (field->field) > > field_name = field->field->name; > > + else if (field->flags & HIST_FIELD_FL_LOG2) > > + field_name = hist_field_name(field->operands[0], ++level); > > > > if (field_name == NULL) > > field_name = ""; > > @@ -357,8 +365,20 @@ static void hist_trigger_elt_comm_init(struct tracing_map_elt *elt) > > .elt_init = hist_trigger_elt_comm_init, > > }; > > > > -static void destroy_hist_field(struct hist_field *hist_field) > > +static void destroy_hist_field(struct hist_field *hist_field, > > + unsigned int level) > > { > > + unsigned int i; > > + > > + if (level > 2) > > + return; > > + > > + if (!hist_field) > > + return; > > + > > + for (i = 0; i < HIST_FIELD_OPERANDS_MAX; i++) > > + destroy_hist_field(hist_field->operands[i], ++level); > > Wouldn't it be 'level + 1' ? > Yeah, we're in a loop here, so definitely. Thanks for catching this. Tom > Thanks, > Namhyung > > > > + > > kfree(hist_field); > > }