From: "Ser, Simon" <simon.ser@intel.com> To: "tvrtko.ursulin@linux.intel.com" <tvrtko.ursulin@linux.intel.com>, "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "chris@chris-wilson.co.uk" <chris@chris-wilson.co.uk> Cc: "igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org> Subject: Re: [igt-dev] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Date: Fri, 24 May 2019 08:27:02 +0000 [thread overview] Message-ID: <e42e51ba1a31db367e6cf3bd2e23914d6886d8e3.camel@intel.com> (raw) In-Reply-To: <b330aca7-2686-3c42-d17e-bdcf0d412b17@linux.intel.com> On Fri, 2019-05-24 at 09:20 +0100, Tvrtko Ursulin wrote: > On 24/05/2019 08:25, Chris Wilson wrote: > > Apparently VLA structs (e.g. struct { int array[count] }) is a gcc > > extension that clang refuses to support as handling memory layout is too > > difficult for it. > > > > Move the on-stack VLA to the heap. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > benchmarks/gem_wsim.c | 146 +++++++++++++++++++++++++++--------------- > > 1 file changed, 95 insertions(+), 51 deletions(-) > > > > diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c > > index e2ffb93a9..0a0032bff 100644 > > --- a/benchmarks/gem_wsim.c > > +++ b/benchmarks/gem_wsim.c > > @@ -1441,6 +1441,48 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask) > > return slice_mask; > > } > > > > +static size_t sizeof_load_balance(int count) > > +{ > > + struct i915_context_engines_load_balance *ptr; > > + > > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); > > This seems wrong - is bound to trigger. > > > + return sizeof(*ptr) + sizeof(ptr->engines[count]); > > So size of of engine needs to be multiplied by count. > > > +} > > + > > +static struct i915_context_engines_load_balance * > > +alloc_load_balance(int count) > > +{ > > + return calloc(1, sizeof_load_balance(count)); > > How about alloca so cleanup is simpler? Or is alloca also on the > unpopular list? > > Or possibly what Simon suggested, just a large temporary stack arrays > would be enough and easiest diff. Just with an assert that it fits. > > I can do that if you want? I think Arek already has a patch for this. > Regards, > > Tvrtko > > > +} > > + > > +static size_t sizeof_param_engines(int count) > > +{ > > + struct i915_context_param_engines *ptr; > > + > > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); > > + return sizeof(*ptr) + sizeof(ptr->engines[count]); > > +} > > + > > +static struct i915_context_param_engines * > > +alloc_param_engines(int count) > > +{ > > + return calloc(1, sizeof_param_engines(count)); > > +} > > + > > +static size_t sizeof_engines_bond(int count) > > +{ > > + struct i915_context_engines_bond *ptr; > > + > > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); > > + return sizeof(*ptr) + sizeof(ptr->engines[count]); > > +} > > + > > +static struct i915_context_engines_bond * > > +alloc_engines_bond(int count) > > +{ > > + return calloc(1, sizeof_engines_bond(count)); > > +} > > + > > static int > > prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) > > { > > @@ -1676,66 +1718,54 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) > > } > > > > if (ctx->engine_map) { > > - I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines, > > - ctx->engine_map_count + 1); > > - I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance, > > - ctx->engine_map_count); > > + struct i915_context_param_engines *set_engines = > > + alloc_param_engines(ctx->engine_map_count + 1); > > + struct i915_context_engines_load_balance *load_balance = > > + alloc_load_balance(ctx->engine_map_count); > > struct drm_i915_gem_context_param param = { > > .ctx_id = ctx_id, > > .param = I915_CONTEXT_PARAM_ENGINES, > > - .size = sizeof(set_engines), > > - .value = to_user_pointer(&set_engines), > > + .size = sizeof_param_engines(ctx->engine_map_count + 1), > > + .value = to_user_pointer(set_engines), > > }; > > + struct i915_context_engines_bond *last = NULL; > > > > if (ctx->wants_balance) { > > - set_engines.extensions = > > - to_user_pointer(&load_balance); > > + set_engines->extensions = > > + to_user_pointer(load_balance); > > > > - memset(&load_balance, 0, sizeof(load_balance)); > > - load_balance.base.name = > > + load_balance->base.name = > > I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE; > > - load_balance.num_siblings = > > + load_balance->num_siblings = > > ctx->engine_map_count; > > > > for (j = 0; j < ctx->engine_map_count; j++) > > - load_balance.engines[j] = > > + load_balance->engines[j] = > > get_engine(ctx->engine_map[j]); > > - } else { > > - set_engines.extensions = 0; > > } > > > > /* Reserve slot for virtual engine. */ > > - set_engines.engines[0].engine_class = > > + set_engines->engines[0].engine_class = > > I915_ENGINE_CLASS_INVALID; > > - set_engines.engines[0].engine_instance = > > + set_engines->engines[0].engine_instance = > > I915_ENGINE_CLASS_INVALID_NONE; > > > > for (j = 1; j <= ctx->engine_map_count; j++) > > - set_engines.engines[j] = > > + set_engines->engines[j] = > > get_engine(ctx->engine_map[j - 1]); > > > > + last = NULL; > > for (j = 0; j < ctx->bond_count; j++) { > > unsigned long mask = ctx->bonds[j].mask; > > - I915_DEFINE_CONTEXT_ENGINES_BOND(bond, > > - __builtin_popcount(mask)); > > - struct i915_context_engines_bond *p = NULL, *prev; > > + struct i915_context_engines_bond *bond = > > + alloc_engines_bond(__builtin_popcount(mask)); > > unsigned int b, e; > > > > - prev = p; > > - p = alloca(sizeof(bond)); > > - assert(p); > > - memset(p, 0, sizeof(bond)); > > - > > - if (j == 0) > > - load_balance.base.next_extension = > > - to_user_pointer(p); > > - else if (j < (ctx->bond_count - 1)) > > - prev->base.next_extension = > > - to_user_pointer(p); > > + bond->base.next_extension = to_user_pointer(last); > > + bond->base.name = I915_CONTEXT_ENGINES_EXT_BOND; > > > > - p->base.name = I915_CONTEXT_ENGINES_EXT_BOND; > > - p->virtual_index = 0; > > - p->master = get_engine(ctx->bonds[j].master); > > + bond->virtual_index = 0; > > + bond->master = get_engine(ctx->bonds[j].master); > > > > for (b = 0, e = 0; mask; e++, mask >>= 1) { > > unsigned int idx; > > @@ -1743,44 +1773,58 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) > > if (!(mask & 1)) > > continue; > > > > - idx = find_engine(&set_engines.engines[1], > > + idx = find_engine(&set_engines->engines[1], > > ctx->engine_map_count, > > e); > > - p->engines[b++] = > > - set_engines.engines[1 + idx]; > > + bond->engines[b++] = > > + set_engines->engines[1 + idx]; > > } > > + > > + last = bond; > > } > > + load_balance->base.next_extension = to_user_pointer(last); > > > > gem_context_set_param(fd, ¶m); > > + > > + while (last) { > > + struct i915_context_engines_bond *next = > > + from_user_pointer(last->base.next_extension); > > + free(last); > > + last = next; > > + } > > + free(load_balance); > > + free(set_engines); > > } else if (ctx->wants_balance) { > > const unsigned int count = num_engines_in_class(VCS); > > - I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance, > > - count); > > - I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines, > > - count + 1); > > + struct i915_context_engines_load_balance *load_balance = > > + alloc_load_balance(count); > > + struct i915_context_param_engines *set_engines = > > + alloc_param_engines(count + 1); > > struct drm_i915_gem_context_param param = { > > .ctx_id = ctx_id, > > .param = I915_CONTEXT_PARAM_ENGINES, > > - .size = sizeof(set_engines), > > - .value = to_user_pointer(&set_engines), > > + .size = sizeof_param_engines(count + 1), > > + .value = to_user_pointer(set_engines), > > }; > > > > - set_engines.extensions = to_user_pointer(&load_balance); > > + set_engines->extensions = to_user_pointer(load_balance); > > > > - set_engines.engines[0].engine_class = > > + set_engines->engines[0].engine_class = > > I915_ENGINE_CLASS_INVALID; > > - set_engines.engines[0].engine_instance = > > + set_engines->engines[0].engine_instance = > > I915_ENGINE_CLASS_INVALID_NONE; > > - fill_engines_class(&set_engines.engines[1], VCS); > > + fill_engines_class(&set_engines->engines[1], VCS); > > > > - memset(&load_balance, 0, sizeof(load_balance)); > > - load_balance.base.name = > > + load_balance->base.name = > > I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE; > > - load_balance.num_siblings = count; > > + load_balance->num_siblings = count; > > > > - fill_engines_class(&load_balance.engines[0], VCS); > > + fill_engines_class(&load_balance->engines[0], VCS); > > > > gem_context_set_param(fd, ¶m); > > + > > + free(set_engines); > > + free(load_balance); > > } > > > > if (wrk->sseu) { > > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
WARNING: multiple messages have this Message-ID (diff)
From: "Ser, Simon" <simon.ser@intel.com> To: "tvrtko.ursulin@linux.intel.com" <tvrtko.ursulin@linux.intel.com>, "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>, "chris@chris-wilson.co.uk" <chris@chris-wilson.co.uk> Cc: "igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org> Subject: Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Date: Fri, 24 May 2019 08:27:02 +0000 [thread overview] Message-ID: <e42e51ba1a31db367e6cf3bd2e23914d6886d8e3.camel@intel.com> (raw) In-Reply-To: <b330aca7-2686-3c42-d17e-bdcf0d412b17@linux.intel.com> On Fri, 2019-05-24 at 09:20 +0100, Tvrtko Ursulin wrote: > On 24/05/2019 08:25, Chris Wilson wrote: > > Apparently VLA structs (e.g. struct { int array[count] }) is a gcc > > extension that clang refuses to support as handling memory layout is too > > difficult for it. > > > > Move the on-stack VLA to the heap. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > benchmarks/gem_wsim.c | 146 +++++++++++++++++++++++++++--------------- > > 1 file changed, 95 insertions(+), 51 deletions(-) > > > > diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c > > index e2ffb93a9..0a0032bff 100644 > > --- a/benchmarks/gem_wsim.c > > +++ b/benchmarks/gem_wsim.c > > @@ -1441,6 +1441,48 @@ set_ctx_sseu(struct ctx *ctx, uint64_t slice_mask) > > return slice_mask; > > } > > > > +static size_t sizeof_load_balance(int count) > > +{ > > + struct i915_context_engines_load_balance *ptr; > > + > > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); > > This seems wrong - is bound to trigger. > > > + return sizeof(*ptr) + sizeof(ptr->engines[count]); > > So size of of engine needs to be multiplied by count. > > > +} > > + > > +static struct i915_context_engines_load_balance * > > +alloc_load_balance(int count) > > +{ > > + return calloc(1, sizeof_load_balance(count)); > > How about alloca so cleanup is simpler? Or is alloca also on the > unpopular list? > > Or possibly what Simon suggested, just a large temporary stack arrays > would be enough and easiest diff. Just with an assert that it fits. > > I can do that if you want? I think Arek already has a patch for this. > Regards, > > Tvrtko > > > +} > > + > > +static size_t sizeof_param_engines(int count) > > +{ > > + struct i915_context_param_engines *ptr; > > + > > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); > > + return sizeof(*ptr) + sizeof(ptr->engines[count]); > > +} > > + > > +static struct i915_context_param_engines * > > +alloc_param_engines(int count) > > +{ > > + return calloc(1, sizeof_param_engines(count)); > > +} > > + > > +static size_t sizeof_engines_bond(int count) > > +{ > > + struct i915_context_engines_bond *ptr; > > + > > + assert(sizeof(ptr->engines[count]) == count * sizeof(ptr->engines[0])); > > + return sizeof(*ptr) + sizeof(ptr->engines[count]); > > +} > > + > > +static struct i915_context_engines_bond * > > +alloc_engines_bond(int count) > > +{ > > + return calloc(1, sizeof_engines_bond(count)); > > +} > > + > > static int > > prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) > > { > > @@ -1676,66 +1718,54 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) > > } > > > > if (ctx->engine_map) { > > - I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines, > > - ctx->engine_map_count + 1); > > - I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance, > > - ctx->engine_map_count); > > + struct i915_context_param_engines *set_engines = > > + alloc_param_engines(ctx->engine_map_count + 1); > > + struct i915_context_engines_load_balance *load_balance = > > + alloc_load_balance(ctx->engine_map_count); > > struct drm_i915_gem_context_param param = { > > .ctx_id = ctx_id, > > .param = I915_CONTEXT_PARAM_ENGINES, > > - .size = sizeof(set_engines), > > - .value = to_user_pointer(&set_engines), > > + .size = sizeof_param_engines(ctx->engine_map_count + 1), > > + .value = to_user_pointer(set_engines), > > }; > > + struct i915_context_engines_bond *last = NULL; > > > > if (ctx->wants_balance) { > > - set_engines.extensions = > > - to_user_pointer(&load_balance); > > + set_engines->extensions = > > + to_user_pointer(load_balance); > > > > - memset(&load_balance, 0, sizeof(load_balance)); > > - load_balance.base.name = > > + load_balance->base.name = > > I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE; > > - load_balance.num_siblings = > > + load_balance->num_siblings = > > ctx->engine_map_count; > > > > for (j = 0; j < ctx->engine_map_count; j++) > > - load_balance.engines[j] = > > + load_balance->engines[j] = > > get_engine(ctx->engine_map[j]); > > - } else { > > - set_engines.extensions = 0; > > } > > > > /* Reserve slot for virtual engine. */ > > - set_engines.engines[0].engine_class = > > + set_engines->engines[0].engine_class = > > I915_ENGINE_CLASS_INVALID; > > - set_engines.engines[0].engine_instance = > > + set_engines->engines[0].engine_instance = > > I915_ENGINE_CLASS_INVALID_NONE; > > > > for (j = 1; j <= ctx->engine_map_count; j++) > > - set_engines.engines[j] = > > + set_engines->engines[j] = > > get_engine(ctx->engine_map[j - 1]); > > > > + last = NULL; > > for (j = 0; j < ctx->bond_count; j++) { > > unsigned long mask = ctx->bonds[j].mask; > > - I915_DEFINE_CONTEXT_ENGINES_BOND(bond, > > - __builtin_popcount(mask)); > > - struct i915_context_engines_bond *p = NULL, *prev; > > + struct i915_context_engines_bond *bond = > > + alloc_engines_bond(__builtin_popcount(mask)); > > unsigned int b, e; > > > > - prev = p; > > - p = alloca(sizeof(bond)); > > - assert(p); > > - memset(p, 0, sizeof(bond)); > > - > > - if (j == 0) > > - load_balance.base.next_extension = > > - to_user_pointer(p); > > - else if (j < (ctx->bond_count - 1)) > > - prev->base.next_extension = > > - to_user_pointer(p); > > + bond->base.next_extension = to_user_pointer(last); > > + bond->base.name = I915_CONTEXT_ENGINES_EXT_BOND; > > > > - p->base.name = I915_CONTEXT_ENGINES_EXT_BOND; > > - p->virtual_index = 0; > > - p->master = get_engine(ctx->bonds[j].master); > > + bond->virtual_index = 0; > > + bond->master = get_engine(ctx->bonds[j].master); > > > > for (b = 0, e = 0; mask; e++, mask >>= 1) { > > unsigned int idx; > > @@ -1743,44 +1773,58 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags) > > if (!(mask & 1)) > > continue; > > > > - idx = find_engine(&set_engines.engines[1], > > + idx = find_engine(&set_engines->engines[1], > > ctx->engine_map_count, > > e); > > - p->engines[b++] = > > - set_engines.engines[1 + idx]; > > + bond->engines[b++] = > > + set_engines->engines[1 + idx]; > > } > > + > > + last = bond; > > } > > + load_balance->base.next_extension = to_user_pointer(last); > > > > gem_context_set_param(fd, ¶m); > > + > > + while (last) { > > + struct i915_context_engines_bond *next = > > + from_user_pointer(last->base.next_extension); > > + free(last); > > + last = next; > > + } > > + free(load_balance); > > + free(set_engines); > > } else if (ctx->wants_balance) { > > const unsigned int count = num_engines_in_class(VCS); > > - I915_DEFINE_CONTEXT_ENGINES_LOAD_BALANCE(load_balance, > > - count); > > - I915_DEFINE_CONTEXT_PARAM_ENGINES(set_engines, > > - count + 1); > > + struct i915_context_engines_load_balance *load_balance = > > + alloc_load_balance(count); > > + struct i915_context_param_engines *set_engines = > > + alloc_param_engines(count + 1); > > struct drm_i915_gem_context_param param = { > > .ctx_id = ctx_id, > > .param = I915_CONTEXT_PARAM_ENGINES, > > - .size = sizeof(set_engines), > > - .value = to_user_pointer(&set_engines), > > + .size = sizeof_param_engines(count + 1), > > + .value = to_user_pointer(set_engines), > > }; > > > > - set_engines.extensions = to_user_pointer(&load_balance); > > + set_engines->extensions = to_user_pointer(load_balance); > > > > - set_engines.engines[0].engine_class = > > + set_engines->engines[0].engine_class = > > I915_ENGINE_CLASS_INVALID; > > - set_engines.engines[0].engine_instance = > > + set_engines->engines[0].engine_instance = > > I915_ENGINE_CLASS_INVALID_NONE; > > - fill_engines_class(&set_engines.engines[1], VCS); > > + fill_engines_class(&set_engines->engines[1], VCS); > > > > - memset(&load_balance, 0, sizeof(load_balance)); > > - load_balance.base.name = > > + load_balance->base.name = > > I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE; > > - load_balance.num_siblings = count; > > + load_balance->num_siblings = count; > > > > - fill_engines_class(&load_balance.engines[0], VCS); > > + fill_engines_class(&load_balance->engines[0], VCS); > > > > gem_context_set_param(fd, ¶m); > > + > > + free(set_engines); > > + free(load_balance); > > } > > > > if (wrk->sseu) { > > > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev
next prev parent reply other threads:[~2019-05-24 8:27 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-24 7:25 [PATCH i-g-t] benchmarks/gem_wsim: Heap allocate VLA structs Chris Wilson 2019-05-24 7:25 ` [igt-dev] " Chris Wilson 2019-05-24 7:45 ` Ser, Simon 2019-05-24 7:45 ` Ser, Simon 2019-05-24 7:50 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork 2019-05-24 8:20 ` [PATCH i-g-t] " Tvrtko Ursulin 2019-05-24 8:20 ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin 2019-05-24 8:27 ` Ser, Simon [this message] 2019-05-24 8:27 ` Ser, Simon 2019-05-24 8:33 ` Chris Wilson 2019-05-24 8:33 ` [igt-dev] [Intel-gfx] " Chris Wilson 2019-05-24 8:39 ` [igt-dev] " Ser, Simon 2019-05-24 8:39 ` [igt-dev] [Intel-gfx] " Ser, Simon 2019-05-24 8:44 ` Tvrtko Ursulin 2019-05-24 8:44 ` [Intel-gfx] " Tvrtko Ursulin 2019-05-24 8:45 ` [PATCH i-g-t v2] benchmarks/gem_wsim: Manually calculate VLA struct sizes Chris Wilson 2019-05-24 8:45 ` [igt-dev] " Chris Wilson 2019-05-24 9:35 ` Tvrtko Ursulin 2019-05-24 9:35 ` [igt-dev] [Intel-gfx] " Tvrtko Ursulin 2019-05-24 11:07 ` [igt-dev] ✓ Fi.CI.BAT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2) Patchwork 2019-05-25 13:11 ` [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs Patchwork 2019-05-25 16:10 ` [igt-dev] ✓ Fi.CI.IGT: success for benchmarks/gem_wsim: Heap allocate VLA structs (rev2) Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e42e51ba1a31db367e6cf3bd2e23914d6886d8e3.camel@intel.com \ --to=simon.ser@intel.com \ --cc=chris@chris-wilson.co.uk \ --cc=igt-dev@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=tvrtko.ursulin@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.