`On Wed, Oct 20, 2021 at 8:48 AM Steven Rostedt <rostedt@goodmis.org> wrote:>> On Tue, 19 Oct 2021 18:31:40 -0700> Kalesh Singh <kaleshsingh@google.com> wrote:>> > @@ -2391,60 +2460,61 @@ static int check_expr_operands(struct trace_array *tr,> >  static struct hist_field *parse_expr(struct hist_trigger_data *hist_data,> >                                    struct trace_event_file *file,> >                                    char *str, unsigned long flags,> > -                                  char *var_name, unsigned int level)> > +                                  char *var_name, unsigned int *n_subexprs)> >  {> >       struct hist_field *operand1 = NULL, *operand2 = NULL, *expr = NULL;> >       unsigned long operand_flags;> >       int field_op, ret = -EINVAL;> >       char *sep, *operand1_str;> >> > -     if (level > 3) {> > +     if (*n_subexprs > 3) {>> Why limit the sub expressions, and not just keep the limit of the level of> recursion. We allow 3 levels of recursion, but we could have more than 3> sub expressions.>>> If we have:  a * b + c / d - e * f / h>> It would break down into:>               ->        +            />    *       /     *     h>  a   b   c  d  e  f>>> Which I believe is 6 "sub expressions", but never goes more than three deep> in recursion:>>    "a * b + c / d - e * f / h">> Step 1:>>   op = "-">   operand1 = "a * b + c / d">   operand2 = "e * f / h">> Process operand1: (recursion level 1)>>   op = "+">   operand1a = "a * b">   operand2a = "c / d">> Process operand1a: (recursion level 2)>>   op = "*">   operand1b = "a">   operand2b = "b">> return;>> Process operand1b: (recursion level 2)>>   op = "/">   operand1b = "c">   operand2b = "d">> return;>> return;>> Process operand2: (recursion level 1)>>   op = "/">   operand1c = "e * f">   operand2c = "h">> Process operand1c: (recursion level 2)>>   op = "*">   operand1c = "e">   operand2c = "f">> return;>> return;>>>> > +> > +     /* LHS of string is an expression e.g. a+b in a+b+c */> > +     operand1 = parse_expr(hist_data, file, operand1_str, operand_flags, NULL, n_subexprs);> >       if (IS_ERR(operand1)) {> >               ret = PTR_ERR(operand1);> >               operand1 = NULL;>> I wonder if we should look for optimizations, in case of operand1 and> operand2 are both constants?>> Just perform the function, and convert it into a constant as well.I think we achieve something like this by propagating up theHIST_FIELD_FL_CONST flag. Thanks for the suggestions. I'll update thisin the next version.Thanks,Kalesh>> -- Steve`