Re: [PATCH i-g-t 05/10] gem_wsim: Support random buffer sizes

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 17/06/2020 17:31, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-06-17 17:01:15)
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> > See README for more details.
> >
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> >   benchmarks/gem_wsim.c  | 71 +++++++++++++++++++++++++++++++++---------
> >   benchmarks/wsim/README |  4 +++
> >   2 files changed, 61 insertions(+), 14 deletions(-)
> >
> > diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> > index 5893de38a98e..c1405596c46a 100644
> > --- a/benchmarks/gem_wsim.c
> > +++ b/benchmarks/gem_wsim.c
> > @@ -117,12 +117,18 @@ struct bond {
> >          enum intel_engine_id master;
> >   };
> >   
> > +struct work_buffer_size {
> > +       unsigned long size;
> > +       unsigned long min;
> > +       unsigned long max;
> > +};
> > +
> >   struct working_set {
> >          int id;
> >          bool shared;
> >          unsigned int nr;
> >          uint32_t *handles;
> > -       unsigned long *sizes;
> > +       struct work_buffer_size *sizes;
> >   };
> >   
> >   struct workload;
> > @@ -203,6 +209,7 @@ struct workload
> >          bool print_stats;
> >   
> >          uint32_t bb_prng;
> > +       uint32_t bo_prng;
> >   
> >          struct timespec repeat_start;
> >   
> > @@ -757,10 +764,12 @@ static int add_buffers(struct working_set *set, char *str)
> >           * 4m
> >           * 4g
> >           * 10n4k - 10 4k batches
> > +        * 4096-16k - random size in range
> >           */
> > -       unsigned long *sizes, size;
> > +       struct work_buffer_size *sizes;
> > +       unsigned long min_sz, max_sz;
> > +       char *n, *max = NULL;
> >          unsigned int add, i;
> > -       char *n;
> >   
> >          n = index(str, 'n');
> >          if (n) {
> > @@ -773,16 +782,34 @@ static int add_buffers(struct working_set *set, char *str)
> >                  add = 1;
> >          }
> >   
> > -       size = parse_size(str);
> > -       if (!size)
> > +       n = index(str, '-');
> > +       if (n) {
> > +               *n = 0;
> > +               max = ++n;
> > +       }
> > +
> > +       min_sz = parse_size(str);
> > +       if (!min_sz)
> >                  return -1;
> >   
> > +       if (max) {
> > +               max_sz = parse_size(max);
> > +               if (!max_sz)
> > +                       return -1;
> > +       } else {
> > +               max_sz = min_sz;
> > +       }
> > +
> >          sizes = realloc(set->sizes, (set->nr + add) * sizeof(*sizes));
> >          if (!sizes)
> >                  return -1;
> >   
> > -       for (i = 0; i < add; i++)
> > -               sizes[set->nr + i] = size;
> > +       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;
> > +       }
> >   
> >          set->nr += add;
> >          set->sizes = sizes;
> > @@ -824,7 +851,7 @@ static uint64_t engine_list_mask(const char *_str)
> >          return mask;
> >   }
> >   
> > -static void allocate_working_set(struct working_set *set);
> > +static void allocate_working_set(struct workload *wrk, struct working_set *set);
> >   
> >   #define int_field(_STEP_, _FIELD_, _COND_, _ERR_) \
> >          if ((field = strtok_r(fstart, ".", &fctx))) { \
> > @@ -1177,10 +1204,12 @@ add_step:
> >   
> >          wrk->nr_steps = nr_steps;
> >          wrk->steps = steps;
> > +       wrk->flags = flags;
> >          wrk->prio = arg->prio;
> >          wrk->sseu = arg->sseu;
> >          wrk->max_working_set_id = -1;
> >          wrk->working_sets = NULL;
> > +       wrk->bo_prng = (wrk->flags & SYNCEDCLIENTS) ? master_prng : rand();
> >   
> >          free(desc);
> >   
> > @@ -1234,7 +1263,7 @@ add_step:
> >           */
> >          for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
> >                  if (w->type == WORKINGSET && w->working_set.shared)
> > -                       allocate_working_set(&w->working_set);
> > +                       allocate_working_set(wrk, &w->working_set);
> >          }
> >   
> >          wrk->max_working_set_id = -1;
> > @@ -1267,6 +1296,7 @@ clone_workload(struct workload *_wrk)
> >          igt_assert(wrk);
> >          memset(wrk, 0, sizeof(*wrk));
> >   
> > +       wrk->flags = _wrk->flags;
> >          wrk->prio = _wrk->prio;
> >          wrk->sseu = _wrk->sseu;
> >          wrk->nr_steps = _wrk->nr_steps;
>
> wrk->flags wasn't introduced in this patch, why are we needing to copy
> them now.
>
> I see wrk->bo_prn = flags & SYNC above, but I haven't seem them used
> again later. They used to carry the balancer info and were setup in
> main. Am I not mistaken in thinking they still are being set in main()
> as well?

I couldn't remember but looking around looks like you are right. I'll do 
some experiments and remove it if confirmed.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx