From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751451AbeCLNm1 (ORCPT ); Mon, 12 Mar 2018 09:42:27 -0400 Received: from mail.kernel.org ([198.145.29.99]:48044 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751242AbeCLNm0 (ORCPT ); Mon, 12 Mar 2018 09:42:26 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 20122205F4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Mon, 12 Mar 2018 22:42:22 +0900 From: Masami Hiramatsu To: Steven Rostedt Cc: linux-kernel@vger.kernel.org, Ingo Molnar , Andrew Morton , Al Viro , Tom Zanussi , Namhyung Kim , Masami Hiramatsu , Jiri Olsa Subject: Re: [PATCH 2/3] tracing: Clean up and document pred_funcs_##type creation and use Message-Id: <20180312224222.da7bd18944b1e4649e53c247@kernel.org> In-Reply-To: <20180310023907.664302683@goodmis.org> References: <20180310023442.791997138@goodmis.org> <20180310023907.664302683@goodmis.org> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.31; x86_64-redhat-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 Fri, 09 Mar 2018 21:34:44 -0500 Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > The pred_funcs_##type arrays consist of five functions that are assigned > based on the ops. The array must be in the same order of the ops each > function represents. The PRED_FUNC_START macro denotes the op enum that > starts the op that maps to the pred_funcs_##type arrays. This is all very > subtle and prone to bugs if the code is changed. > > Add comments describing how PRED_FUNC_START and pred_funcs_##type array is > used, and also a PRED_FUNC_MAX that is the maximum number of functions in > the arrays. > > Clean up select_comparison_fn() that assigns the predicates to the > pred_funcs_##type array function as well as add protection in case an op is > passed in that does not map correctly to the array. > This looks good to me. Reviewed-by: Masami Hiramatsu Thanks, > Signed-off-by: Steven Rostedt (VMware) > --- > kernel/trace/trace_events_filter.c | 46 ++++++++++++++++++++++++++------------ > 1 file changed, 32 insertions(+), 14 deletions(-) > > diff --git a/kernel/trace/trace_events_filter.c b/kernel/trace/trace_events_filter.c > index a2ef393b3bb2..9d383f4383dc 100644 > --- a/kernel/trace/trace_events_filter.c > +++ b/kernel/trace/trace_events_filter.c > @@ -65,6 +65,13 @@ struct filter_op { > > static struct filter_op filter_ops[] = { OPS }; > > +/* > + * pred functions are OP_LT, OP_LE, OP_GT, OP_GE, and OP_BAND > + * pred_funcs_##type below must match the order of them above. > + */ > +#define PRED_FUNC_START OP_LT > +#define PRED_FUNC_MAX (OP_BAND - PRED_FUNC_START) > + > #define ERRORS \ > C( NONE, "No error"), \ > C( INVALID_OP, "Invalid operator"), \ > @@ -172,8 +179,6 @@ static const filter_pred_fn_t pred_funcs_##type[] = { \ > filter_pred_BAND_##type, \ > }; > > -#define PRED_FUNC_START OP_LT > - > #define DEFINE_EQUALITY_PRED(size) \ > static int filter_pred_##size(struct filter_pred *pred, void *event) \ > { \ > @@ -946,39 +951,52 @@ static filter_pred_fn_t select_comparison_fn(enum filter_op_ids op, > int field_size, int field_is_signed) > { > filter_pred_fn_t fn = NULL; > + int pred_func_index = -1; > + > + switch (op) { > + case OP_EQ: > + case OP_NE: > + break; > + default: > + if (WARN_ON_ONCE(op < PRED_FUNC_START)) > + return NULL; > + pred_func_index = op - PRED_FUNC_START; > + if (WARN_ON_ONCE(pred_func_index > PRED_FUNC_MAX)) > + return NULL; > + } > > switch (field_size) { > case 8: > - if (op == OP_EQ || op == OP_NE) > + if (pred_func_index < 0) > fn = filter_pred_64; > else if (field_is_signed) > - fn = pred_funcs_s64[op - PRED_FUNC_START]; > + fn = pred_funcs_s64[pred_func_index]; > else > - fn = pred_funcs_u64[op - PRED_FUNC_START]; > + fn = pred_funcs_u64[pred_func_index]; > break; > case 4: > - if (op == OP_EQ || op == OP_NE) > + if (pred_func_index < 0) > fn = filter_pred_32; > else if (field_is_signed) > - fn = pred_funcs_s32[op - PRED_FUNC_START]; > + fn = pred_funcs_s32[pred_func_index]; > else > - fn = pred_funcs_u32[op - PRED_FUNC_START]; > + fn = pred_funcs_u32[pred_func_index]; > break; > case 2: > - if (op == OP_EQ || op == OP_NE) > + if (pred_func_index < 0) > fn = filter_pred_16; > else if (field_is_signed) > - fn = pred_funcs_s16[op - PRED_FUNC_START]; > + fn = pred_funcs_s16[pred_func_index]; > else > - fn = pred_funcs_u16[op - PRED_FUNC_START]; > + fn = pred_funcs_u16[pred_func_index]; > break; > case 1: > - if (op == OP_EQ || op == OP_NE) > + if (pred_func_index < 0) > fn = filter_pred_8; > else if (field_is_signed) > - fn = pred_funcs_s8[op - PRED_FUNC_START]; > + fn = pred_funcs_s8[pred_func_index]; > else > - fn = pred_funcs_u8[op - PRED_FUNC_START]; > + fn = pred_funcs_u8[pred_func_index]; > break; > } > > -- > 2.15.1 > > -- Masami Hiramatsu