From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1949473AbcBSVX1 (ORCPT ); Fri, 19 Feb 2016 16:23:27 -0500 Received: from smtprelay0042.hostedemail.com ([216.40.44.42]:38395 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1947850AbcBSVXW (ORCPT ); Fri, 19 Feb 2016 16:23:22 -0500 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::,RULES_HIT:2:41:355:379:541:599:960:966:973:988:989:1042:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1535:1593:1594:1605:1730:1747:1777:1792:2196:2199:2393:2538:2553:2559:2562:2692:2693:2911:3138:3139:3140:3141:3142:3622:3855:3865:3866:3867:3868:3870:3871:3872:3873:3874:4049:4118:4250:4321:4385:4425:4605:5007:6119:6261:6755:7809:7875:7903:7904:8603:8957:9040:10004:10848:10967:11026:11232:11473:11658:11914:12043:12219:12295:12296:12438:12517:12519:12555:12663:12740:13138:13161:13180:13229:13231:14096:14097:14659:21080:21088:21220:30054:30067:30074:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: neck72_9183cfabfaa56 X-Filterd-Recvd-Size: 7818 Date: Fri, 19 Feb 2016 16:23:18 -0500 From: Steven Rostedt To: Tom Zanussi 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 Subject: Re: [PATCH v13 09/29] tracing: Add 'hist' event trigger command Message-ID: <20160219162318.10de33fe@gandalf.local.home> In-Reply-To: <15dc2e4792676582404b398c020926c412c63dcb.1449767187.git.tom.zanussi@linux.intel.com> References: <15dc2e4792676582404b398c020926c412c63dcb.1449767187.git.tom.zanussi@linux.intel.com> X-Mailer: Claws Mail 3.13.1 (GTK+ 2.24.29; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? > + 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. 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. Is that suppose to be a full match or partial match? > + 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. > + 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. -- Steve