Re: [igt-dev] [PATCH i-g-t 6/8] benchmarks/gem_wsim: allow comments in workload description files

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/09/2023 17:20, Bernatowicz, Marcin wrote:
> 
> 
> On 9/21/2023 5:22 PM, Tvrtko Ursulin wrote:
>>
>> On 21/09/2023 16:05, Bernatowicz, Marcin wrote:
>>> Hi,
>>>
>>> On 9/20/2023 6:13 PM, Tvrtko Ursulin wrote:
>>>>
>>>> On 06/09/2023 16:51, Marcin Bernatowicz wrote:
>>>>> Lines starting with '#' are skipped.
>>>>> If command line step separator (',') is encountered after '#'
>>>>> it is replaced with ';' to not break parsing.
>>>>>
>>>>> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
>>>>> ---
>>>>>   benchmarks/gem_wsim.c  | 41 
>>>>> ++++++++++++++++++++++++++++++++---------
>>>>>   benchmarks/wsim/README |  2 ++
>>>>>   2 files changed, 34 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>>>>> index 0c1b58727..ec9fdc2d0 100644
>>>>> --- a/benchmarks/gem_wsim.c
>>>>> +++ b/benchmarks/gem_wsim.c
>>>>> @@ -43,6 +43,7 @@
>>>>>   #include <limits.h>
>>>>>   #include <pthread.h>
>>>>>   #include <math.h>
>>>>> +#include <ctype.h>
>>>>>   #include "drm.h"
>>>>>   #include "drmtest.h"
>>>>> @@ -94,6 +95,7 @@ enum w_type {
>>>>>       TERMINATE,
>>>>>       SSEU,
>>>>>       WORKINGSET,
>>>>> +    SKIP,
>>>>>   };
>>>>>   struct dep_entry {
>>>>> @@ -930,6 +932,12 @@ parse_workload(struct w_arg *arg, unsigned int 
>>>>> flags, double scale_dur,
>>>>>           if (field) {
>>>>>               fstart = NULL;
>>>>> +            /* line starting with # is a comment */
>>>>> +            if (field[0] == '#') {
>>>>> +                step.type = SKIP;
>>>>> +                goto add_step;
>>>>> +            }
>>>>
>>>> Do they need to be recorded as steps and couldn't be simply silently 
>>>> skipped over while parsing?
>>>
>>> Looks indeed a bool skip may be enough. It's a dummy step not stored 
>>> in workload steps.
>>
>> Cool, that would be the best.
>>
>>>>
>>>> How does relative step referencing works when comments are present? 
>>>> (Batch implicit dependencies and 'a' and 's' commands.)
>>>
>>> Comments may be added to existing workloads without a problem as the 
>>> SKIPs are not incrementing nr_step (does not create a record).
>>
>> Oookay, guess I was confused with the goto add_step. :)
>>
>>> Maybe a small confusion is that step.idx does not correspond to line 
>>> number, so in case of failed parse we get a step number without a 
>>> line :/
>>
>> Hm? If parse fails the whole program fails so I didn't get this.
> 
> Sorry, I created a confusion. A Failure message does not point the line 
> number but the step number (0-based) - that didn't change.
> Looking at workload definition (broken for example) in text editor 
> having line number could easy a bit:
> 
> 1  # Workload simulating transcoding session
> 2  # ex. gem_wsim -w benchmarks/wsim/media_load_balance_fhd26u7.wsim -c 
> 36 -r 600
> 3  # will run 36 parallel transcoding session streams for 600 frames each
> 4  M.3.VCS
> 5  B.3
> 6  1.VCS1.1200-1800.0.0
> 7  1.VCS1.1900-2100.0.0
> 8  2.RCS.1500-2000.-1.0
> 9  3.VCS.1400-1800.-1.1
> 10 1.VCS1.1900-2100.-1.0
> 11 2.RCS.1500-2000.-1.0
> 12 3.VCS.1400-1800.-1.1
> 13 1.VCS1.1900-2100.-1.0
> 14 2.RCS.200-400.-1.0
> 15 2.RCS.1500-2000.0.0
> 16 3.VCS.1400-1800.-1.1
> 17 1.VCS1.1900-2100.-1.0
> 18 2.RCS.1500-2000.-1.0
> 19 3.VCS.1400-1800.-1.1
> 20 1.VCS1.1900-2100.-1.0
> 21 2.RCS.200-400.-1.0
> 22 2.RCS.1500-2000.0.0
> 23 3.VCS.1400-k1800.-1.1
> 24 1.VCS1.1900-2100.-1.0
> 25 2.RCS.1500-2000.-1.0
> 26 3.VCS.1400-1800.-1.1
> 27 1.VCS1.1900-2100.-1.0
> 28 2.RCS.1500-2000.-1.0
> 29 2.RCS.1500-2000.0.0
> 30 3.VCS.1400-1800.-1.1
> 
> Current parse failure message will tell:
> 
> Invalid duration at step 19!
> Failed to parse workload 0!
> 
> Which was on line 20 (step no + 1) and now is on 23 (+ no of comment 
> lines).
> But there are more important matters :)

Ah right.. yes it would be handy to have better parsing error reporting. 
I think it is easy to do. Just store line numbers in struct w_step. 
Heck, we could strdup the whole line while parsing and go super fancy by 
showing it verbatim on error, like:

Invalid duration at step 19, line 23!
    >>> '23 3.VCS.1400-k1800.-1.1'
Failed to parse workload 0!

Regards,

Tvrtko

> 
> Regards,
> Marcin
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>> Marcin
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> +
>>>>>               if (!strcmp(field, "d")) {
>>>>>                   int_field(DELAY, delay, tmp <= 0,
>>>>>                         "Invalid delay at step %u!\n");
>>>>> @@ -1194,7 +1202,8 @@ parse_workload(struct w_arg *arg, unsigned 
>>>>> int flags, double scale_dur,
>>>>>           if (field) {
>>>>>               fstart = NULL;
>>>>> -            check_arg(strlen(field) != 1 ||
>>>>> +            check_arg(!strlen(field) ||
>>>>> +                  (strlen(field) > 1 && !isspace(field[1]) && 
>>>>> field[1] != '#') ||
>>>>>                     (field[0] != '0' && field[0] != '1'),
>>>>>                     "Invalid wait boolean at step %u!\n",
>>>>>                     nr_steps);
>>>>> @@ -1208,18 +1217,23 @@ parse_workload(struct w_arg *arg, unsigned 
>>>>> int flags, double scale_dur,
>>>>>           step.type = BATCH;
>>>>>   add_step:
>>>>> -        if (step.type == DELAY)
>>>>> -            step.delay = __duration(step.delay, scale_time);
>>>>> +        if (step.type == SKIP) {
>>>>> +            if (verbose > 3)
>>>>> +                printf("skipped STEP: %s\n", _token);
>>>>> +        } else {
>>>>> +            if (step.type == DELAY)
>>>>> +                step.delay = __duration(step.delay, scale_time);
>>>>> -        step.idx = nr_steps++;
>>>>> -        step.request = -1;
>>>>> -        steps = realloc(steps, sizeof(step) * nr_steps);
>>>>> -        igt_assert(steps);
>>>>> +            step.idx = nr_steps++;
>>>>> +            step.request = -1;
>>>>> +            steps = realloc(steps, sizeof(step) * nr_steps);
>>>>> +            igt_assert(steps);
>>>>> -        memcpy(&steps[nr_steps - 1], &step, sizeof(step));
>>>>> +            memcpy(&steps[nr_steps - 1], &step, sizeof(step));
>>>>> +        }
>>>>>           free(token);
>>>>> -    }
>>>>> +    } // while ((_token = strtok_r(tstart, ",", &tctx))) {
>>>>>       if (app_w) {
>>>>>           steps = realloc(steps, sizeof(step) *
>>>>> @@ -2304,6 +2318,8 @@ static void *run_workload(void *data)
>>>>>               enum intel_engine_id engine = w->engine;
>>>>>               int do_sleep = 0;
>>>>> +            igt_assert(w->type != SKIP);
>>>>> +
>>>>>               if (w->type == DELAY) {
>>>>>                   do_sleep = w->delay;
>>>>>               } else if (w->type == PERIOD) {
>>>>> @@ -2543,6 +2559,13 @@ static char *load_workload_descriptor(char 
>>>>> *filename)
>>>>>       close(infd);
>>>>>       for (i = 0; i < len; i++) {
>>>>> +        /* '#' starts comment till end of line */
>>>>> +        if (buf[i] == '#')
>>>>> +            /* replace ',' in comments to not break parsing */
>>>>> +            while (++i < len && buf[i] != '\n')
>>>>> +                if (buf[i] == ',')
>>>>> +                    buf[i] = ';';
>>>>> +
>>>>>           if (buf[i] == '\n')
>>>>>               buf[i] = ',';
>>>>>       }
>>>>> diff --git a/benchmarks/wsim/README b/benchmarks/wsim/README
>>>>> index 8c71f2fe6..e4fd61645 100644
>>>>> --- a/benchmarks/wsim/README
>>>>> +++ b/benchmarks/wsim/README
>>>>> @@ -1,6 +1,8 @@
>>>>>   Workload descriptor format
>>>>>   ==========================
>>>>> +Lines starting with '#' are treated as comments (do not create 
>>>>> work step).
>>>>> +
>>>>>   ctx.engine.duration_us.dependency.wait,...
>>>>>   <uint>.<str>.<uint>[-<uint>]|*.<int <= 0>[/<int <= 
>>>>> 0>][...].<0|1>,...
>>>>>   B.<uint>