From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1427805AbcBSXtO (ORCPT ); Fri, 19 Feb 2016 18:49:14 -0500 Received: from mga04.intel.com ([192.55.52.120]:5970 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1427316AbcBSXtM (ORCPT ); Fri, 19 Feb 2016 18:49:12 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,473,1449561600"; d="scan'208";a="749677350" Message-ID: <1455924470.4713.30.camel@tzanussi-mobl.amr.corp.intel.com> Subject: Re: [PATCH v13 09/29] tracing: Add 'hist' event trigger command From: Tom Zanussi To: Steven Rostedt Cc: masami.hiramatsu.pt@hitachi.com, namhyung@kernel.org, josh@joshtriplett.org, andi@firstfloor.org, mathieu.desnoyers@efficios.com, peterz@infradead.org, linux-kernel@vger.kernel.org Date: Fri, 19 Feb 2016 17:27:50 -0600 In-Reply-To: <20160219162318.10de33fe@gandalf.local.home> References: <15dc2e4792676582404b398c020926c412c63dcb.1449767187.git.tom.zanussi@linux.intel.com> <20160219162318.10de33fe@gandalf.local.home> 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 Steve, On Fri, 2016-02-19 at 16:23 -0500, Steven Rostedt wrote: > On Thu, 10 Dec 2015 12:50:51 -0600 > Tom Zanussi wrote: > > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > > new file mode 100644 > > index 0000000..213aa79 > > --- /dev/null > > +++ b/kernel/trace/trace_events_hist.c > > @@ -0,0 +1,837 @@ > > +/* > > + * trace_events_hist - trace event hist triggers > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * Copyright (C) 2015 Tom Zanussi > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "tracing_map.h" > > +#include "trace.h" > > + > > +struct hist_field; > > + > > +typedef u64 (*hist_field_fn_t) (struct hist_field *field, void *event); > > + > > +struct hist_field { > > + struct ftrace_event_field *field; > > + unsigned long flags; > > + hist_field_fn_t fn; > > + unsigned int size; > > +}; > > + > > +static u64 hist_field_counter(struct hist_field *field, void *event) > > +{ > > + return 1; > > +} > > + > > +static u64 hist_field_string(struct hist_field *hist_field, void *event) > > +{ > > + char *addr = (char *)(event + hist_field->field->offset); > > + > > + return (u64)(unsigned long)addr; > > +} > > + > > +#define DEFINE_HIST_FIELD_FN(type) \ > > +static u64 hist_field_##type(struct hist_field *hist_field, void *event)\ > > +{ \ > > + type *addr = (type *)(event + hist_field->field->offset); \ > > + \ > > + return (u64)*addr; \ > > +} > > + > > +DEFINE_HIST_FIELD_FN(s64); > > +DEFINE_HIST_FIELD_FN(u64); > > +DEFINE_HIST_FIELD_FN(s32); > > +DEFINE_HIST_FIELD_FN(u32); > > +DEFINE_HIST_FIELD_FN(s16); > > +DEFINE_HIST_FIELD_FN(u16); > > +DEFINE_HIST_FIELD_FN(s8); > > +DEFINE_HIST_FIELD_FN(u8); > > + > > +#define for_each_hist_field(i, hist_data) \ > > + for (i = 0; i < hist_data->n_fields; i++) > > + > > +#define for_each_hist_val_field(i, hist_data) \ > > + for (i = 0; i < hist_data->n_vals; i++) > > + > > +#define for_each_hist_key_field(i, hist_data) \ > > + for (i = hist_data->n_vals; i < hist_data->n_fields; i++) > > The above should have parenthesis around the macro variables. > > e.g. > > (hist_data)->n_fields > > > + > > +#define HITCOUNT_IDX 0 > > +#define HIST_KEY_MAX 1 > > +#define HIST_KEY_SIZE_MAX MAX_FILTER_STR_VAL > > + > > +enum hist_field_flags { > > + HIST_FIELD_HITCOUNT = 1, > > + HIST_FIELD_KEY = 2, > > + HIST_FIELD_STRING = 4, > > Note, please add "_FL" to the enums to denote they are flags (used for > bitmask). > > e.g. HIST_FIELD_FL_HITCOUNT > > > > +}; > > + > > +struct hist_trigger_attrs { > > + char *keys_str; > > + unsigned int map_bits; > > +}; > > + > > +struct hist_trigger_data { > > + struct hist_field *fields[TRACING_MAP_FIELDS_MAX]; > > Hmm I'm confused, TRACING_MAP_FIELDS_MAX == 4 > > But below it we index into fields with n_fields. Which look to me can > be much bigger than 4. > > Or is there some correlation between n_vals, n_keys and 4? > Yeah, the correlation is: n_fields = n_vals + n_keys where n_keys can't be larger than TRACING_MAP_KEYS_MAX, and n_vals can't be larger than TRACING_MAP_FIELDS_MAX - TRACING_MAP_KEYS_MAX, where currently TRACING_MAP_KEYS_MAX = 2 and TRACING_MAP_FIELDS_MAX = 4, which means the max number of values is currently 2 as well. So n_fields shouldn't be bigger than 4, but... Looking through the code to make sure, I realized that it should be fields[TRACING_MAP_FIELDS_MAX + 1] because of the hitcount, which is an always-defined value field and takes the first slot in fields[] (followed by the rest of the vals, then the keys). Previous versions had separate keys[] and vals[] arrays, but that was changed to be more uniform and somehow adding 1 for the hitcount was overlooked. Obviously I'll fix that size, but the rest of the code should be ok wrt that. > > + unsigned int n_vals; > > + unsigned int n_keys; > > + unsigned int n_fields; > > + unsigned int key_size; > > + struct tracing_map_sort_key sort_keys[TRACING_MAP_SORT_KEYS_MAX]; > > + unsigned int n_sort_keys; > > + struct trace_event_file *event_file; > > + struct hist_trigger_attrs *attrs; > > + struct tracing_map *map; > > +}; > > + > > +static hist_field_fn_t select_value_fn(int field_size, int field_is_signed) > > +{ > > + hist_field_fn_t fn = NULL; > > + > > + switch (field_size) { > > + case 8: > > + if (field_is_signed) > > + fn = hist_field_s64; > > + else > > + fn = hist_field_u64; > > + break; > > + case 4: > > + if (field_is_signed) > > + fn = hist_field_s32; > > + else > > + fn = hist_field_u32; > > + break; > > + case 2: > > + if (field_is_signed) > > + fn = hist_field_s16; > > + else > > + fn = hist_field_u16; > > + break; > > + case 1: > > + if (field_is_signed) > > + fn = hist_field_s8; > > + else > > + fn = hist_field_u8; > > + break; > > + } > > + > > + return fn; > > +} > > + > > +static int parse_map_size(char *str) > > +{ > > + unsigned long size, map_bits; > > + int ret; > > + > > + strsep(&str, "="); > > + if (!str) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + ret = kstrtoul(str, 0, &size); > > + if (ret) > > + goto out; > > + > > + map_bits = ilog2(roundup_pow_of_two(size)); > > + if (map_bits < TRACING_MAP_BITS_MIN || > > + map_bits > TRACING_MAP_BITS_MAX) > > + ret = -EINVAL; > > + else > > + ret = map_bits; > > + out: > > + return ret; > > +} > > + > > +static void destroy_hist_trigger_attrs(struct hist_trigger_attrs *attrs) > > +{ > > + if (!attrs) > > + return; > > + > > + kfree(attrs->keys_str); > > + kfree(attrs); > > +} > > + > > +static struct hist_trigger_attrs *parse_hist_trigger_attrs(char *trigger_str) > > +{ > > + struct hist_trigger_attrs *attrs; > > + int ret = 0; > > + > > + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL); > > + if (!attrs) > > + return ERR_PTR(-ENOMEM); > > + > > + while (trigger_str) { > > + char *str = strsep(&trigger_str, ":"); > > + > > + if (!strncmp(str, "keys", strlen("keys")) || > > + !strncmp(str, "key", strlen("key"))) > > nit, I prefer "strncmp(x,y,n) == 0" as it shows an equals, and, at > least to me, easier to quickly understand. > OK, I'll change it - I've gotten so used to automatically doing the negation in my head, but I don't care either way. > But not so nit, if !strncmp(str, "keys", strlen("keys")) then > !strcmp(str, "key", strlen("key")) is true! > > It has the same result as: > > if (!strncmp(str, "key", strlen("key"))) > > That is equivalent to if (x > 12 || x > 10) where > if (x > 10) will do. > Yeah, that is kind of useless. > Is that suppose to be a full match or partial match? > It should match either but not less or more. > > + attrs->keys_str = kstrdup(str, GFP_KERNEL); > > + else if (!strncmp(str, "size", strlen("size"))) { > > Is it OK to allow "sizeUgaBooga=10"? > > Perhaps the compare should be against "size=" ? > > Same for the keys above. Yeah, good point, I'll fix all those up.. > > > + int map_bits = parse_map_size(str); > > + > > + if (map_bits < 0) { > > + ret = map_bits; > > + goto free; > > + } > > + attrs->map_bits = map_bits; > > + } else { > > + ret = -EINVAL; > > + goto free; > > + } > > + } > > + > > + if (!attrs->keys_str) { > > + ret = -EINVAL; > > + goto free; > > + } > > + > > + return attrs; > > + free: > > + destroy_hist_trigger_attrs(attrs); > > + > > + return ERR_PTR(ret); > > +} > > + > > I stopped here. I'll review the rest later. > OK, thanks! Tom > -- Steve