From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 980BC10E59F for ; Wed, 27 Sep 2023 19:03:57 +0000 (UTC) Message-ID: <5a0bbd8d-5637-3fde-7d16-b5120e052794@linux.intel.com> Date: Wed, 27 Sep 2023 21:03:51 +0200 MIME-Version: 1.0 Content-Language: en-US To: Tvrtko Ursulin , igt-dev@lists.freedesktop.org References: <20230926084451.1732748-1-marcin.bernatowicz@linux.intel.com> <20230926084451.1732748-8-marcin.bernatowicz@linux.intel.com> From: "Bernatowicz, Marcin" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [igt-dev] [PATCH i-g-t 07/14] benchmarks/gem_wsim: cleanups List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: chris.p.wilson@linux.intel.com Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 9/26/2023 1:08 PM, Tvrtko Ursulin wrote: > > On 26/09/2023 09:44, Marcin Bernatowicz wrote: >> Cleaning checkpatch.pl reported warnings/errors. >> Removed unused fence_signal field from struct w_step. >> calloc vs malloc in parse_workload for struct workload. >> >> Signed-off-by: Marcin Bernatowicz >> --- >>   benchmarks/gem_wsim.c | 56 ++++++++++++++++++++++++++----------------- >>   1 file changed, 34 insertions(+), 22 deletions(-) >> >> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c >> index 3b01340bf..daa20fb8a 100644 >> --- a/benchmarks/gem_wsim.c >> +++ b/benchmarks/gem_wsim.c >> @@ -1,3 +1,4 @@ >> +// SPDX-License-Identifier: MIT >>   /* >>    * Copyright © 2017 Intel Corporation >>    * >> @@ -76,8 +77,7 @@ struct duration { >>       bool unbound_duration; >>   }; >> -enum w_type >> -{ >> +enum w_type { >>       BATCH, >>       SYNC, >>       DELAY, >> @@ -102,8 +102,7 @@ struct dep_entry { >>       int working_set; /* -1 = step dependecy, >= 0 working set id */ >>   }; >> -struct deps >> -{ >> +struct deps { >>       int nr; >>       bool submit_fence; >>       struct dep_entry *list; >> @@ -137,8 +136,7 @@ struct working_set { >>   struct workload; >> -struct w_step >> -{ >> +struct w_step { >>       struct workload *wrk; >>       /* Workload step metadata */ >> @@ -155,7 +153,6 @@ struct w_step >>           int period; >>           int target; >>           int throttle; >> -        int fence_signal; >>           int priority; >>           struct { >>               unsigned int engine_map_count; >> @@ -194,8 +191,7 @@ struct ctx { >>       uint64_t sseu; >>   }; >> -struct workload >> -{ >> +struct workload { >>       unsigned int id; >>       unsigned int nr_steps; >> @@ -807,6 +803,7 @@ static int add_buffers(struct working_set *set, >> char *str) >>       for (i = 0; i < add; i++) { >>           struct work_buffer_size *sz = &sizes[set->nr + i]; >> + >>           sz->min = min_sz; >>           sz->max = max_sz; >>           sz->size = 0; >> @@ -895,13 +892,16 @@ parse_duration(unsigned int nr_steps, struct >> duration *dur, double scale_dur, ch >>   } >>   #define int_field(_STEP_, _FIELD_, _COND_, _ERR_) \ >> -    if ((field = strtok_r(fstart, ".", &fctx))) { \ >> -        tmp = atoi(field); \ >> -        check_arg(_COND_, _ERR_, nr_steps); \ >> -        step.type = _STEP_; \ >> -        step._FIELD_ = tmp; \ >> -        goto add_step; \ >> -    } \ >> +    do { \ >> +        field = strtok_r(fstart, ".", &fctx); \ >> +        if (field) { \ >> +            tmp = atoi(field); \ >> +            check_arg(_COND_, _ERR_, nr_steps); \ >> +            step.type = _STEP_; \ >> +            step._FIELD_ = tmp; \ >> +            goto add_step; \ >> +        } \ >> +    } while (0) >>   static struct workload * >>   parse_workload(struct w_arg *arg, unsigned int flags, double scale_dur, >> @@ -926,7 +926,8 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>           valid = 0; >>           memset(&step, 0, sizeof(step)); >> -        if ((field = strtok_r(fstart, ".", &fctx))) { >> +        field = strtok_r(fstart, ".", &fctx); >> +        if (field) { >>               fstart = NULL; >>               if (!strcmp(field, "d")) { >> @@ -943,6 +944,7 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>                   } >>               } else if (!strcmp(field, "P")) { >>                   unsigned int nr = 0; >> + >>                   while ((field = strtok_r(fstart, ".", &fctx))) { >>                       tmp = atoi(field); >>                       check_arg(nr == 0 && tmp <= 0, >> @@ -968,6 +970,7 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>                         "Invalid sync target at step %u!\n"); >>               } else if (!strcmp(field, "S")) { >>                   unsigned int nr = 0; >> + >>                   while ((field = strtok_r(fstart, ".", &fctx))) { >>                       tmp = atoi(field); >>                       check_arg(tmp <= 0 && nr == 0, >> @@ -1004,6 +1007,7 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>                   goto add_step; >>               } else if (!strcmp(field, "M")) { >>                   unsigned int nr = 0; >> + >>                   while ((field = strtok_r(fstart, ".", &fctx))) { >>                       tmp = atoi(field); >>                       check_arg(nr == 0 && tmp <= 0, >> @@ -1034,6 +1038,7 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>                         "Invalid terminate target at step %u!\n"); >>               } else if (!strcmp(field, "X")) { >>                   unsigned int nr = 0; >> + >>                   while ((field = strtok_r(fstart, ".", &fctx))) { >>                       tmp = atoi(field); >>                       check_arg(nr == 0 && tmp <= 0, >> @@ -1058,6 +1063,7 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>                   goto add_step; >>               } else if (!strcmp(field, "B")) { >>                   unsigned int nr = 0; >> + >>                   while ((field = strtok_r(fstart, ".", &fctx))) { >>                       tmp = atoi(field); >>                       check_arg(nr == 0 && tmp <= 0, >> @@ -1077,6 +1083,7 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>                   goto add_step; >>               } else if (!strcmp(field, "b")) { >>                   unsigned int nr = 0; >> + >>                   while ((field = strtok_r(fstart, ".", &fctx))) { >>                       check_arg(nr > 2, >>                             "Invalid bond format at step %u!\n", >> @@ -1148,7 +1155,8 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>               valid++; >>           } >> -        if ((field = strtok_r(fstart, ".", &fctx))) { >> +        field = strtok_r(fstart, ".", &fctx); >> +        if (field) { >>               fstart = NULL; >>               i = str_to_engine(field); >> @@ -1160,7 +1168,8 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>               step.engine = i; >>           } >> -        if ((field = strtok_r(fstart, ".", &fctx))) { >> +        field = strtok_r(fstart, ".", &fctx); >> +        if (field) { >>               fstart = NULL; >>               tmp = parse_duration(nr_steps, &step.duration, >> scale_dur, field); >> @@ -1170,7 +1179,8 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>               valid++; >>           } >> -        if ((field = strtok_r(fstart, ".", &fctx))) { >> +        field = strtok_r(fstart, ".", &fctx); >> +        if (field) { >>               fstart = NULL; >>               tmp = parse_dependencies(nr_steps, &step, field); >> @@ -1180,7 +1190,8 @@ parse_workload(struct w_arg *arg, unsigned int >> flags, double scale_dur, >>               valid++; >>           } >> -        if ((field = strtok_r(fstart, ".", &fctx))) { >> +        field = strtok_r(fstart, ".", &fctx); >> +        if (field) { >>               fstart = NULL; >>               check_arg(strlen(field) != 1 || >> @@ -1224,7 +1235,7 @@ add_step: >>           nr_steps += app_w->nr_steps; >>       } >> -    wrk = malloc(sizeof(*wrk)); >> +    wrk = calloc(1, sizeof(*wrk)); > > Rest looks fine but this change I don't know what checkpatch has against > it and why calloc(1,..) is better. That's the kernels checkpatch.pl? I > don't get it when I run it. I'll restore it in next version. My previous patch changes contained fields which were zeroed here, now it's not required and creates confusion, I agree it's not a cleanup/issue and not reported by the any tool. -- marcin > > Regards, > > Tvrtko > >>       igt_assert(wrk); >>       wrk->nr_steps = nr_steps; >> @@ -2717,6 +2728,7 @@ int main(int argc, char **argv) >>       if (append_workload_arg) { >>           struct w_arg arg = { NULL, append_workload_arg, 0 }; >> + >>           app_w = parse_workload(&arg, flags, scale_dur, scale_time, >>                          NULL); >>           if (!app_w) {